-
Notifications
You must be signed in to change notification settings - Fork 137
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
Changes from all commits
77e739a
b9ac048
aeea144
791a27e
40bd95f
0b1408d
984ad9c
672470b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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')], | ||
plugins: [ | ||
[ | ||
resolve('@babel/plugin-transform-typescript'), | ||
{ | ||
allowDeclareFields: true, | ||
onlyRemoveTypeImports: true, | ||
// Default enums are IIFEs | ||
optimizeConstEnums: true, | ||
}, | ||
], | ||
[ | ||
resolve('@babel/plugin-proposal-decorators'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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'), | ||
], | ||
}; |
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([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as an aside, is there anything we can do with |
||
'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({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you have this? Browser support should be an app concern. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the docs confirm it: https://github.com/wessberg/rollup-plugin-ts#babelpreset-env-behavior-and-how-to-opt-out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I have been doing this also here! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
tsconfig: { | ||
// when `hooks` is specified, fileName is required | ||
fileName: 'tsconfig.json', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, I'll remove. thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
], | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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