-
Notifications
You must be signed in to change notification settings - Fork 140
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
New babel config #2016
New babel config #2016
Conversation
9d7c1ef
to
9ad7cc8
Compare
a17f526
to
8c96c9c
Compare
About modifying this block 👉 https://github.com/embroider-build/embroider/blob/main/packages/compat/src/compat-app.ts#L244C1-L255C1 to help developers get how to update their app. I need some rough idea. I understand the value of identifying app's options and addon's options to output good messages, for instance:
But I don't think I can know that using only Is the above worth it or should I try to go with slightly more generic messages? EDIT: discussed with Chris, we will go with the generic message. |
df00ceb
to
5956d5d
Compare
…abel plugins for classic addons, import it in the babel.config.cjs of the app-template to merge explicit and classic plugins
…abel config from generated config to the app config
…ate counterpart and install them in app-template deps
…lates, use an empty config for classic templates
… so the config is more readable
5956d5d
to
788e9d4
Compare
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.
The only state that we need to capture during the prebuild is the plugins that were inserted by v1 addons. Most of our own plugins don't need to be in that legacy plugins list. They can be provided as directly importable utilities in the babel config.
Here is where we add many plugins:
embroider/packages/compat/src/compat-app-builder.ts
Lines 227 to 290 in 9372659
// Our stage3 code is always allowed to use dynamic import. We may emit it | |
// ourself when splitting routes. | |
babel.plugins.push(require.resolve('@babel/plugin-syntax-dynamic-import')); | |
// https://github.com/webpack/webpack/issues/12154 | |
babel.plugins.push(require.resolve('./rename-require-plugin')); | |
babel.plugins.push([ | |
require.resolve('babel-plugin-ember-template-compilation'), | |
await this.etcOptions(resolverConfig), | |
]); | |
// this is @embroider/macros configured for full stage3 resolution | |
babel.plugins.push(...this.compatApp.macrosConfig.babelPluginConfig()); | |
let colocationOptions: TemplateColocationPluginOptions = { | |
appRoot: this.origAppPackage.root, | |
// This extra weirdness is a compromise in favor of build performance. | |
// | |
// 1. When auto-upgrading an addon from v1 to v2, we definitely want to | |
// run any custom AST transforms in stage1. | |
// | |
// 2. In general case, AST transforms are allowed to manipulate Javascript | |
// scope. This means that running transforms -- even when we're doing | |
// source-to-source compilation that emits handlebars and not wire | |
// format -- implies changing .hbs files into .js files. | |
// | |
// 3. So stage1 may need to rewrite .hbs to .hbs.js (to avoid colliding | |
// with an existing co-located .js file). | |
// | |
// 4. But stage1 doesn't necessarily want to run babel over the | |
// corresponding JS file. Most of the time, that's just an | |
// unnecessarily expensive second parse. (We only run it in stage1 to | |
// eliminate an addon's custom babel plugins, and many addons don't | |
// have any.) | |
// | |
// 5. Therefore, the work of template-colocation gets defered until here, | |
// and it may see co-located templates named `.hbs.js` instead of the | |
// usual `.hbs. | |
templateExtensions: ['.hbs', '.hbs.js'], | |
// All of the above only applies to auto-upgraded packages that were | |
// authored in v1. V2 packages don't get any of this complexity, they're | |
// supposed to take care of colocating their own templates explicitly. | |
packageGuard: true, | |
}; | |
babel.plugins.push([templateColocationPluginPath, colocationOptions]); | |
babel.plugins.push([ | |
require.resolve('./babel-plugin-adjust-imports'), | |
(() => { | |
let pluginConfig: AdjustImportsOptions = { | |
appRoot: resolverConfig.appRoot, | |
}; | |
return pluginConfig; | |
})(), | |
]); | |
// we can use globally shared babel runtime by default | |
babel.plugins.push([ | |
require.resolve('@babel/plugin-transform-runtime'), | |
{ absoluteRuntime: __dirname, useESModules: true, regenerator: false }, | |
]); |
- syntax-dynamic-import may not be needed at all, I would be surprised if it's not enabled by default in new-enough babel. If it is needed, it can go directly in the user's config.
- rename-require-plugin is only needed by webpack, so it can be dropped entirely here
- babel-plugin-ember-template-compilation is important for users to be able to control in their own babel config. We will need to work on cleanly generating its options. For now, we could provide an importable utilities that builds the options for you.
- Macros is similar, users should be able to see it in their config, but right now it has a not-user-friendly API, so it could be an importable utility that wraps over that for now.
- template colocation could go directly into the users's config.
- babel-pluign-adjust-imports is a v1-addon rule-system-supporting plugin. I think we should consider renaming
loadLegacyPlugins()
tov1AddonSupport()
and then it mean both the v1-addon-inserted plugins plus any of our own hacks like babel-plugin-adjust-imports that exist purely to support v1 addons. That way users either add or remove all v1 addon support, without needing to study a lot of details.
} | ||
|
||
module.exports = config; | ||
module.exports = {}; |
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.
I think this file can just be deleted.
function _warnIfNoLegacyPlugins(legacyPlugins: any) { | ||
if (!legacyPlugins || !legacyPlugins.length) { | ||
console.warn(` | ||
Your Ember app doesn't use any classic addon that requires Babel plugins. | ||
In your babel.config.cjs, you can safely remove the usage of loadLegacyPlugins. | ||
|
||
Remove: | ||
- const { loadLegacyPlugins } = require('@embroider/compat'); | ||
- ...loadLegacyPlugins(), | ||
|
||
If you install a classic addon in your app afterward, make sure to add any Babel config it may require to babel.config.cjs. | ||
`); |
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.
I worry that this is premature. Long after someone has seen this message and removed the legacy plugin support, someone on their team could add an addon that needs it and be puzzled why the addon doesn't work.
@mansona just suggested to me that perhaps it would be better to wait to tell people to remove the legacy plugin support until we can do a deprecation against any v1 addon trying to add babel plugins, and that sounds reasonable.
@@ -225,21 +225,8 @@ export default class CompatApp { | |||
|
|||
@Memoize() | |||
babelConfig(): TransformOptions { |
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.
This whole method can change name and return type because its only remaining responsibility is to capture the plugins added by v1 addons. It could become captureLegacyBabelPlugins(): { plugins: PluginItem[] }
@@ -581,6 +581,9 @@ export class CompatAppBuilder { | |||
`module.exports = ${JSON.stringify(pconfig.config, null, 2)}`, | |||
'utf8' | |||
); | |||
outputJSONSync(join(locateEmbroiderWorkingDir(this.compatApp.root), '_babel_config_.json'), pconfig.config, { | |||
spaces: 2, | |||
}); |
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.
This is adding a new output location without removing the previous one on the lines above?
Also, similar to my comment in combat-app.ts, this value we're creating no longer needs to be a whole babel config. It's just a file that exports a plugins
array (and eventually it will probably also export an AST transforms array, but that is a separate topic.).
"@babel/plugin-proposal-decorators": "^7.22.5", | ||
"@babel/plugin-transform-class-properties": "7.24.7", | ||
"@babel/plugin-transform-private-methods": "7.24.7", | ||
"@babel/plugin-transform-private-property-in-object": "^7.24.7", |
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.
This is a non-blocking comment, but it's probably safe for us to replace all four of these with https://github.com/ef4/decorator-transforms instead. It could be a separate PR.
I could never finish this PR after the Embroider Initiative stopped, Ed implemented it in #2102. |
Fixes #1962
TODO:
exclude
option with the new config that can achieve whatskipBabel
does.babel.ts
toload-babel-config.ts
to be more explicit it's config loading and not some Babel pluginloadPluginDebugMacros
right afterloadLegacyPlugins
to simplify