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

feat: allow to opt-out of default babel plugins and presets #41

Closed
wants to merge 9 commits into from

Conversation

sastan
Copy link
Contributor

@sastan sastan commented Nov 6, 2019

The unique feature of generating typescript declaration files for all chunks is really great and we want (need) to use this plugin.

But: we are using a own preset that internally configures @babel/preset-env. This can not be detected by rollup-plugin-ts. This PR introduces an option noBabelConfigCustomization to completly disable the default presets and plugins (which are a great feature!). This is opt-in or opt-out depending on the view ;-)

I hope this can be merged and happy to adjust this PR.

@sastan sastan force-pushed the no-default-babel-plugins branch from a1fd26f to db45d32 Compare November 6, 2019 17:30
@tunnckoCore
Copy link

Great! I'm for it too. This was causing problems in the past and in the TSDX try of integrating this plugin.

@sastan sastan force-pushed the no-default-babel-plugins branch from db45d32 to 08b3571 Compare November 6, 2019 21:09
@wessberg
Copy link
Owner

wessberg commented Nov 6, 2019

Hey there. Thanks for your contribution! Yes, I've been thinking along the same lines. So let's get the ball rolling on merging this PR 🙂.

One thought that I have is this: The plugin does some magic to separate minification-related plugins/presets (such as babel-preset-minify and related plugins) from the provided or discovered babel config(s) such that the majority of babel plugins run on a file-by-file basis except for minification-related plugins which then run on a chunk-by-chunk basis. The reason is that you get much better minification this way. And the reason for running everything else on a file-by-file basis is of course to make Rollup/Acorn understand whatever experimental syntax you may be transforming with babel.

Also, there are some pitfalls that are easy to get into that I really, really want to help users avoid. Things like duplicating babel helpers many times across chunks, or the modules option of preset-env and the other yearly presets, which will never work with Rollup and should never have any other value than false.

I still think its valuable to enforce these options, even if rollup-plugin-ts doesn't add any default presets/plugins. That would require some changes to your PR in src/util/get-babel-config/get-babel-config.ts.

I'm thinking along the lines of the following:

  • Provide an option that can disable any default babel plugins/presets
  • Move babel plugins and presets into optional dependencies (everything except @babel/core which is needed to resolve and manipulate config files).

How do you feel about that?

@sastan sastan force-pushed the no-default-babel-plugins branch from 08b3571 to 801fbd7 Compare November 7, 2019 08:20
@sastan
Copy link
Contributor Author

sastan commented Nov 7, 2019

separate minification-related plugins/presets

included in updated PR

This is done using loadOptions which returns a flat resolved list of plugins, that is then matched against the configured names.

As we do not know which plugins may get loaded for a filename hasMinifyOptions is always set to true. That currently results in some plugins applied twice (renderChunk and transform). Maybe we could change which plugins are selected during renderChunk (minifyConfig). I would suggest to only include those that have been excluded during transform or are syntax plugins (babel-plugin-syntax-* or just [-/]syntax-*). What do you think?

modules option of preset-env and the other yearly presets

included in updated PR

I included the caller api which is used by preset-env to detect if it can emit ES Modules. The user should use modules: 'auto' (the default value of preset-env). modules: 'auto' allows to use the same babel config for different transpile targets like rollup (esm) and jest (cjs) and it just works.

Alternatively modules: false would work aswell.

Move babel plugins and presets into optional dependencies

included in updated PR

That would mean they get installed anyway. I think they should move into peerDependencies which would ensure only semver compatible versions are installed. Some package managers support peerDependenciesMeta (yarn, pnpm and npm@ ^6.11.0 ) which allow users to not install those peerDependencies.

{
  "peerDependencies": {
    "@babel/plugin-transform-runtime": "^7.6.2"
  },
  "peerDependenciesMeta": {
    "@babel/plugin-transform-runtime": {
      "optional": true
    }
  }
}

de-duplicating babel helpers

pending solution

A very good feature. We have @babel/transform-runtime configured in our preset aswell. Our configuration differs between projects. All do use the helpers option and some enable corejs and regenerator. useESModules is always determined by api.caller (same way as @babel/preset-env/src/index.js).

Maybe we only ensure that @babel/transform-runtime is included and helpers is set to true. What do you think?

Copy link
Owner

@wessberg wessberg left a comment

Choose a reason for hiding this comment

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

I added a few comments :-)

package.json Outdated
Comment on lines 86 to 91
"@babel/plugin-proposal-async-generator-functions": "^7.2.0",
"@babel/plugin-proposal-json-strings": "^7.2.0",
"@babel/plugin-proposal-object-rest-spread": "^7.6.2",
"@babel/plugin-proposal-optional-catch-binding": "^7.2.0",
"@babel/plugin-proposal-unicode-property-regex": "^7.6.2",
"@babel/plugin-syntax-dynamic-import": "^7.2.0",
Copy link
Owner

Choose a reason for hiding this comment

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

it turns out that @babel/preset-env already depends on all of the proposal-* plugins that rollup-plugin-ts depends on, so we can leave all of these out.

package.json Outdated
Comment on lines 100 to 117
"@babel/plugin-proposal-async-generator-functions": {
"optional": true
},
"@babel/plugin-proposal-json-strings": {
"optional": true
},
"@babel/plugin-proposal-object-rest-spread": {
"optional": true
},
"@babel/plugin-proposal-optional-catch-binding": {
"optional": true
},
"@babel/plugin-proposal-unicode-property-regex": {
"optional": true
},
"@babel/plugin-syntax-dynamic-import": {
"optional": true
},
Copy link
Owner

Choose a reason for hiding this comment

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

it turns out that @babel/preset-env already depends on all of the proposal-* plugins that rollup-plugin-ts depends on, so we can leave these out.

@wessberg
Copy link
Owner

wessberg commented Nov 9, 2019

separate minification-related plugins/presets

As we do not know which plugins may get loaded for a filename hasMinifyOptions is always set to true. That currently results in some plugins applied twice (renderChunk and transform). Maybe we could change which plugins are selected during renderChunk (minifyConfig). I would suggest to only include those that have been excluded during transform or are syntax plugins (babel-plugin-syntax-* or just [-/]syntax-*). What do you think?

Yes, agreed. Basically, it's not so much minification vs not, but rather "what should run per-file" vs "what should run per chunk". Mangling, for example, is definitely better per chunk than per file. So, for starters, I think it makes sense to stop calling it minifyOptions and start calling it chunkConfig or something like that.

I agree with you on the syntax-plugins-plus-whatever-was-excluded-during-transform point.

modules option of preset-env and the other yearly presets

I included the caller api which is used by preset-env to detect if it can emit ES Modules. The user should use modules: 'auto' (the default value of preset-env). modules: 'auto' allows to use the same babel config for different transpile targets like rollup (esm) and jest (cjs) and it just works.

Sounds great!

de-duplicating babel helpers

Maybe we only ensure that @babel/transform-runtime is included and helpers is set to true. What do you think?

Yes, due to the reasons outlined here and like known from other plugins like this, I very strongly believe it is a responsibility of this plugin to avoid helper functions being duplicated. That is the very reason for depending on @babel/plugin-transform-runtime. There have been few issues such as reported here when users have depended on conflicting versions of @babel/plugin-transform-runtime, and I've even considered moving the helpers into the rollup-plugin-ts codebase itself to avoid such incompatibilities, but that comes with the price of needing to maintain them.

We may end up doing something like that, but for now, my suggestion is to keep forcing the plugin, like we do with tslib.

Move babel plugins and presets into optional dependencies

That would mean they get installed anyway. I think they should move into peerDependencies which would ensure only semver compatible versions are installed.

Originally my concern was that it would be one hell of a npm install command:

npm install @wessberg/rollup-plugin-ts @babel/preset-env @babel/plugin-proposal-async-generator-functions @babel/plugin-proposal-json-strings @babel/plugin-proposal-object-rest-spread @babel/plugin-proposal-optional-catch-binding @babel/plugin-proposal-unicode-property-regex @babel/plugin-syntax-dynamic-import @babel/plugin-transform-runtime

But, it turns out that @babel/preset-env already depends on all of the proposal-* plugins that rollup-plugin-ts depends on, so I've just pushed a commit to the master branch that removes all redundant babel dependencies and also suggested that we remove them from your PR.

@sastan sastan force-pushed the no-default-babel-plugins branch 2 times, most recently from b308d53 to bc1e04d Compare November 10, 2019 20:32
@sastan
Copy link
Contributor Author

sastan commented Nov 12, 2019

Sorry for the delay. I pushed a commit to remove the additional plugins.

This is my plan:

  • minifyOptions / chunkConfig: Going to implement it as suggested but keep the naming or make the default of chunkConfig the value of minifyOptions for compatibility.

  • @babel/plugin-transform-runtime:

    • ensure only one version is used
    • ensure helpers are referenced as imports to allow rollup to chunk them
    • ensure the referenced issues are solved by the solution

I hope i can get something done by thursday.

@sastan sastan force-pushed the no-default-babel-plugins branch 2 times, most recently from 6ed57cc to 7032f08 Compare November 14, 2019 09:48
@sastan
Copy link
Contributor Author

sastan commented Nov 14, 2019

separate minification-related plugins/presets

included in updated PR

  • renamed to chunk*
  • on file transformation chunk plugins/presets are excluded
  • on chunk transformation only syntax and chunk plugins/presets are included

Move ´@babel/preset-envintopeerDependencies`

included in updated PR

  • marked as optional via peerDependenciesMeta

de-duplicating babel helpers

included in updated PR

The current PR ensures that @babel/plugin-transform-runtime is included. If the user has included the plugin, the defaults (FORCED_BABEL_PLUGIN_TRANSFORM_RUNTIME_OPTIONS) are merged with the user options which always have a higher precedence. For example setting helpers to false the user options would disable helper imports. I choose to do it that way because of:

  1. if @babel/plugin-transform-runtime is not included in the user config, the defaults are applied
  2. noBabelConfigCustomization is an advanced option and the user should now what they are doing
  3. overriding one option and not another maybe confusing

Todo

  • update documentation with hint that @babel/preset-env must be installed manually
  • maybe move @babel/plugin-transform-runtime into peerDependencies
  • document and explicit hint about the @babel/plugin-transform-runtime option merging

@sastan sastan force-pushed the no-default-babel-plugins branch from 7032f08 to 2464c56 Compare November 29, 2019 07:41
@sastan
Copy link
Contributor Author

sastan commented Nov 29, 2019

I rebased to PR and removed @babel/preset-env from peerDependencies to prevent breaking changes for users.

@wessberg Is there anything i can to do to get this PR merged?

@wessberg
Copy link
Owner

wessberg commented Nov 29, 2019

Great work. I'm testing out your PR, but I'm running into a crash:

First, in the function configItemIsChunkRelated that now takes a string:

function configItemIsChunkRelated(id: string): boolean {

Sometimes, the function is called with not an ID, but a ConfigItem, as can be seen in the following screenshot. This may lead to crashes.

Screen Shot 2019-11-29 at 8 05 53 PM

Currently on master, it takes a IBabelConfigItem:

function configItemIsMinificationRelated({file: {resolved}}: IBabelConfigItem): boolean {

It would seem that there's some normalization that needs to happen there.

@sastan
Copy link
Contributor Author

sastan commented Dec 5, 2019

Sorry for the delay. I tested only the new config.

Thanks for the detailed report.

Now it should work.

@wessberg
Copy link
Owner

wessberg commented Nov 17, 2021

Hey there. Sorry for the very late response, but the code base has diverged significantly since this PR was originally created. However, the good news is that the general ideas behind this PR has been integrated with rollup-plugin-ts in the new v2.0.0 release, albeit with some changes.

  • All Babel related dependencies are now optional peer dependencies
  • And, @babel/preset-env won't be applied as a preset if the browserslist: false option is passed to the plugin.

Because of this, I'm closing this PR. Thanks for your great work.

@wessberg wessberg closed this Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants