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

Explore ways to avoid creating dependency cycles when plugins add require/import, and generally controling transformation #8754

Open
loganfsmyth opened this issue Sep 22, 2018 · 14 comments

Comments

@loganfsmyth
Copy link
Member

loganfsmyth commented Sep 22, 2018

Feature Request

Compilation of node_modules is becoming more common as more packages are publish as ES6.

This is causing problems in two ways:

  • transform-runtime can potentially insert references to core-js into core-js itself
  • transform-runtime can potentially insert references to @babel/runtime-corejs2 into core-js itself
  • transform-runtime can potentially insert references to @babel/runtime-corejs2 into itself, causing dependency cycles
  • And similar with useBuiltins: 'usage' transforming core-js.

Similarly, things are difficult here because the primary fix would be something like

ignore: [ /core-js/, /@babel\/runtime/ ]

and that is problematic because core-js as a package should theoretically be an implementation detail. For instance, core-js@3 has also created core-js-pure which we might want to use, and if users did ignore: ["**/core-js"] instead or something, they'd be depending in the implementation detail of the exact dependency name. That may not actually be an issue though.

There's kind of two intertwined questions here:

  • Can we autodetect cycles in simple cases? It's not always easy because it could be indirectly cyclic, like core-js -> @babel/runtime-corejs2 -> core-js.
  • Should we instead consider a way for files to opt out of certain transformations?

I don't think we'd want to have a way to 100% opt out of plugin processing, but we could for instance consider a pragma that allowed you to opt out of some list of names. The biggest pain point being typeof-symbol since typeof is an existing ES5 feature, it is easy to get cycles since core-js itself obviously has to use it.

We could for instance consider a directive like

"no compile(typeof-symbol)";

or something that polyfills could put in their entry point, or just "no compile"; to opt out entirely. I'm imagining that would apply only for plugins that actually opt in themselves, so while all of our standard plugins could opt out, random third-party plugins would still have to make that decision on their own, which is fine I think.

We've had people ask for this type of thing in the past anyway, so I'd be interested. Also, we could follow the standard directive rules, so you could also opt a single function out of compiling or something. We could also support "compile(classes)"; as a way to say only compile classes?

Examples:

@loganfsmyth loganfsmyth changed the title Explore ways to avoid creating dependency cycles when plugins add require/import Explore ways to avoid creating dependency cycles when plugins add require/import, and generally controling transformation Sep 22, 2018
@loganfsmyth
Copy link
Member Author

@ljharb Would a directive like this be something you'd be interested in for polyfill usage?

@ljharb
Copy link
Member

ljharb commented Sep 22, 2018

I’d probably use it if it was necessary, but i still don’t understand why we’d want to further enable the terrible practice of transforming code you didn’t author.

@timdp
Copy link

timdp commented Sep 22, 2018

@ljharb I don't disagree with the sentiment, but do you then expect package maintainers to supply a build for every conceivable runtime environment? This is why preset-env accepts a browserslist.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

By default, i expect them to only provide es5 CJS - if they wish to provide anything else, they’re free to do so.

@timdp
Copy link

timdp commented Sep 23, 2018

Sorry, but you have to be more pragmatic here.

Take the example of async. We use fast-async in our browser bundles because it's lightweight. If I were to import an ES5 module that depends on the Regenerator runtime, my bundle size would explode and my customers wouldn't be happy. I think it's only fair that third parties can use async in their code and let me as a dependent deal with that.

CommonJS has a similar problem. Rollup, for example, relies on ESM and transpiles CJS to ESM via a plugin. However, it does that via the AST. That means that things like module.exports = buildExports() confuse it, even though it's perfectly valid CJS.

The larger question is how a dependency should communicate to its dependents what entry points are available and what language features they require. Rollup uses the package.json module field to define an entry point using ESM alongside main. I think that's a good start, and I wouldn't be against an extension of it.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

@timdp certainly if you want to have a caveat that consumers need a polyfill/shim or similar, you can put that in the readme - as for bundle size, every bundler can transparently alias modules, so there’s no reason bundle size would explode. The important thing is that things work by default, not that the bundle is a few k smaller by default.

As far as “confuse it”, if the bundler you’re using is so easily confused that it can’t handle the majority of CJS (which statically requires and exports), then I’d suggest finding a bundler that understands the language better. Treeshaking, for example, works just as well with CJS - it’s just that bundlers haven’t tended to implement it.

Using nonstandard package.json fields causes long term problems in the ecosystem - for example, the “module” field can now never be used by node since it’s been tainted by various bundlers/packages in the ecosystem, and if whatever ESM system lands in node doesn’t use that field? Then everything using it is instantly tech debt.

@timdp
Copy link

timdp commented Sep 23, 2018

Okay, so take delay, a super popular package. How do you treeshake that without evaluating the code?

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

You don’t, because there’s nothing to shake out - every part of the module is necessary. Treeshaking only provides a benefit when modules are inefficiently architected in the first place.

@timdp
Copy link

timdp commented Sep 23, 2018

Well, no. You don't need delay.reject in all cases, so you want to treeshake that out. Conversely, you want to be able to import { reject as delayReject } from 'delay'.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

since that’s CJS, you need it to be the primary export - and even of the code itself you’re only talking about one line, which isn’t much of a benefit. If you want to be able to get them separately, they’d need to be exported separately - ie, from native ESM, or from two separate modules.

@timdp
Copy link

timdp commented Sep 23, 2018

That's all well and good, but I don't maintain the package and there's no official guidance for package maintainers that would actually support a request to change the code.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

File an issue, make a PR, and fork, in that order, if you find the maintainer is unwilling to maintain, or to accept your change.

@timdp
Copy link

timdp commented Sep 23, 2018

Don't you think that's the same argument you made about package.json module? Fragmentation due to lack of standardization and guidance?

Additionally, my point about async still stands. It's not realistic to ask a package maintainer to supply an ES7 build, a Regenerator build, a fast-async build and God knows what else. That'd be duplicating a lot of effort.

@wmertens
Copy link

So as I understand it, the current solution/workaround is to not process node_modules, and only whitelist certain ones that provide ES modules, and then still run the risk of circular dependencies for those, plus run the risk you're shipping code that's not compatible with some browser?

dmnsgn added a commit to dmnsgn/snowdev that referenced this issue Mar 26, 2021
- ignore core-js and @babel/runtime to avoid circular dependencies (babel/babel#8754)
- use default modules ("auto") for preset-env, add bugfixes (will be default in Babel 8) and set corejs minor version
- add babel transform runtime preset and switch rollup bableHelpers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants