Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typecsript sample rollup config + exploration of different plugins' side-effects #1099

27 changes: 27 additions & 0 deletions packages/addon-dev/sample-typescript-babel.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const { resolve } = require;

module.exports = {
presets: [resolve('@babel/preset-typescript')],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using resolve here, because I hate "plugin not found errors" in babel. Typos are much easier to catch earlier, and resolve helps out with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both @babel/preset-typescript and @babel/plugin-transform-typescript? I think it should be one or the other. The only thing the preset does is install that plugin.

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugin-transform-typescript is required when doing resolve in the babel.config.js (by eslint + node:recommended)
if I just use strings for the plugin names, the issue is side-stepped.

happy to do whatever you think is best here

plugins: [
[
resolve('@babel/plugin-transform-typescript'),
{
allowDeclareFields: true,
onlyRemoveTypeImports: true,
// Default enums are IIFEs
optimizeConstEnums: true,
},
],
[
resolve('@babel/plugin-proposal-decorators'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to specify the class-properties plugin.

By only specifying the decorators plugin, we leave properties "native" and untranspiled, since all browsers support class properties now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is fine, but your explanation for why it's fine is incorrect:

since all browsers support class properties now.

Browser support has nothing to do with it. That is a concern for apps, not addons.

The only reason we need to transpile the decorators here is that acorn doesn't parse them, so rollup blows up. As soon as that is fixed, we should drop decorator transpilation, and that won't depend on whether any browsers have implemented decorators.

{
// The stage 1 implementation
legacy: true,
},
],
[resolve('@babel/plugin-proposal-class-properties')],
resolve('@embroider/addon-dev/template-colocation-plugin'),
],
};
91 changes: 91 additions & 0 deletions packages/addon-dev/sample-typescript-rollup.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import ts from 'rollup-plugin-ts';
import { defineConfig } from 'rollup';

import { Addon } from '@embroider/addon-dev/rollup';

const addon = new Addon({
srcDir: 'src',
destDir: 'dist',
});

export default defineConfig({
// https://github.com/rollup/rollup/issues/1828
watch: {
chokidar: {
usePolling: true,
},
},
output: {
...addon.output(),
sourcemap: true,
// Until a bug with the ember-cli-htmlbars-removal transform is fixed, you
// may need
// hoistTransitiveImports: false
// specified here as well
},
plugins: [
// These are the modules that users should be able to import from your
// addon. Anything not listed here may get optimized away.
addon.publicEntrypoints([
'index.ts',
'components/**/*.ts',
'helpers/**/*.ts',
'modifiers/**/*.ts',
'services/**/*.ts',
'test-support/**/*.ts',
]),

// These are the modules that should get reexported into the traditional
// "app" tree. Things in here should also be in publicEntrypoints above, but
// not everything in publicEntrypoints necessarily needs to go here.
addon.appReexports([
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an aside, is there anything we can do with appReexports and publicEntrypoints to either not need file extensions, or have some sort of assertions on over-specification of extensions? The plugins are very forgiving here, and I'm not sure what's "proper".

'index.js',
'components/**/*.js',
'helpers/**/*.js',
'modifiers/**/*.js',
'services/**/*.js',
'test-support/**/*.js',
]),
// This babel config should *not* apply presets or compile away ES modules.
// It exists only to provide development niceties for you, like automatic
// template colocation.
// See `babel.config.json` for the actual Babel configuration!
ts({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we also want to provide a sample tsconfig?

I've been using:

{
  "compilerOptions": {
    // Path resolution
    "baseUrl": "./src",
    "moduleResolution": "node",

    // types are built by rollup-plugin-ts
    "declarationDir": "./dist",
    "emitDeclarationOnly": true,
    "declaration": true,

    // Build settings
    "noEmitOnError": false,
    "module": "ESNext",
    "target": "ESNext",

    // Features
    "experimentalDecorators": true,
    "allowJs": false,
    "allowSyntheticDefaultImports": false,

    // Strictness / Correctness
    "strict": true,
    "noImplicitAny": true,
    "noImplicitThis": true,
    "alwaysStrict": true,
    "strictNullChecks": true,
    "strictPropertyInitialization": true,
    "noFallthroughCasesInSwitch": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "paths": {

      // No paths, no absolute imports, only node_modules imports allowed
      // But fallback for type-overrides and such
      "*": ["types/*"]
    }
  },
  "include": [
    "src/**/*",
    "types/*"
  ]
}

// can be changed to swc or other transpilers later
// but we need the ember plugins converted first
// (template compilation and co-location)
transpiler: 'babel',
// without the browserlist, recently landed JS features are polyfilled.
// It should be primarily up to the consuming app whether or not they need to
// polyfill whatever JS features.
browserslist: ['last 2 firefox versions', 'last 2 chrome versions'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have this? Browser support should be an app concern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do need this -- the default is "something isn't close to what the browsers could natively support".

Without the browserslist,

  • argument-destructuring is polyfilled
  • nullish coalescing is polyfilled
  • optional chaining is polyfilled
  • etc

I agree that It absolutely should be an app concern to compile away these features, but I think addons should ship "as close to native" as they want to

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's definitely wrong to have this browserslist here. This implies that babel-preset-env is running. We need it not to run.

I think browserslist: false may disable babel-preset-env entirely, just based on perusing the source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have been doing this also here!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting browserslist to false added poplyfills for ownKeys and objectSpread in my addon.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ef4 @NullVoxPopuli As discussed yesterday, I looked into this today. Our suspicion, that rollup-plugin-ts does some unexpected magic, seems to be right unfortunately. I filed this issue: wessberg/rollup-plugin-ts#189

tsconfig: {
// when `hooks` is specified, fileName is required
fileName: 'tsconfig.json',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we omit this, as this is just declaring what is already the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, I'll remove. thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just kidding, it's required when hooks are present

hook: (config) => ({
...config,
declaration: true,
declarationMap: true,
// See: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#beta-delta
// Allows us to use `exports` to define types per export
// However, we can't use that feature until the minimum supported TS is 4.7+
declarationDir: './dist',
}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this hook thing? Most of the options are already set in your tsconfig.json example, except for declarationMap, which we also could set there!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah -- kinda more for understandability than anything -- having the rollup config control all output is easier to reason about than having two places where output can be configured

},
}),

// Follow the V2 Addon rules about dependencies. Your code can import from
// `dependencies` and `peerDependencies` as well as standard Ember-provided
// package names.
addon.dependencies(),

// Ensure that standalone .hbs files are properly integrated as Javascript.
addon.hbs(),

// addons are allowed to contain imports of .css files, which we want rollup
// to leave alone and keep in the published output.
addon.keepAssets(['**/*.css']),

addon.clean(),
],
});