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

Automatically add module condition to default ones #2417

Closed
Andarist opened this issue Jul 26, 2022 · 9 comments
Closed

Automatically add module condition to default ones #2417

Andarist opened this issue Jul 26, 2022 · 9 comments

Comments

@Andarist
Copy link

As a package author, I'm trying to juggle a lot of requirements and keep it compatible with different tools. This is a mad man's world and, for now, we've decided to use { module: './esm-path.js', default: './cjs-path.js' } in a set of our packages.

However, this isn't ideal for esbuild users - our intention is for bundlers to pick up the module condition. This is how both webpack and Rollup work (without an explicit opt-in to this condition on the users' side). I think that Bun is also following the suit but I didn't recheck this right now so I might be mistaken. Is there a chance that esbuid could do the same?

Somewhat related discussions:
#2173
#2163
#1956

I understand that you might want to follow the node's spec - but from my PoV that spec was designed mainly for node and it only defines how node should handle this field. It was deliberately designed in an extensible way though, giving other tools freedom to extend the semantics, adding new conditions etc. There was even an idea to create a centralized place where semantics for different new conditions could be defined as this is currently hidden in the implementations & docs of specific bundlers, even if the semantics are mostly shared between them (if only they use the same condition names, but tools try to do that and new conditions are not added on a whim).

Since esbuild is also a bundler I think it would make sense for it to behave in a similar way as other popular bundlers. Is there any particular thing that you are concerned about in relation to that condition?

@evanw
Copy link
Owner

evanw commented Jul 26, 2022

But why are you doing this? Why not use the standard require and/or import conditions?

@Andarist
Copy link
Author

We are generating exports maps automatically and, for instance, it's a hard sell for us to allow for a dual package hazard. We don't control dependencies of the bundled/built packages and we certainly can't control how the built packages will be consumed.

People might also want to start using exports maps within a minor version and adding an import condition might be a breaking change as other packages might already assume that import def from 'lib' imports the whole namespace~ object and not the transpiled default export. By introducing an import condition this might change for the consumers of a said 'lib'.

In general, we believe that exports + import conditions have a lot of gotchas. While we understand them, this is certainly not true for the majority of users. We don't want our consumers to end up with bizarre and cryptic problems because of how we've decided to structure fields in their package.json files. From our point of view, the usage of exports, import condition and type: 'module' should always be a conscious decision. We plan to provide different "modes" for library packaging but all of them have certain tradeoffs, we've failed to figure out an universal package.json that would "just work" for all the use cases when people want to provide both cjs and esm versions of their libs and when they also want to be able to load any dependency (at this point, I'm pretty certain that it's just impossible). So we've concluded that if somebody opts-into exports maps then this shape is the safest one that we can generate. Any additional "level" of opt-in comes with a new set of constraints/tradeoffs.

One could say that in such a case we shouldn't be using exports at all... but we need a safe-ish way to opt into them to provide a worker condition for bundlers.

@evanw
Copy link
Owner

evanw commented Jul 26, 2022

What you are asking for will likely cause pain for users regarding default exports. If you import CommonJS into ESM then things mostly work as expected (both in esbuild and node) with module.exports being assigned to the default import and potentially property names of module.exports showing up as top-level named imports. However, importing ESM into CommonJS does not work as expected because users sometimes expect require() to return the default export but it actually returns the entire module namespace exotic object.

Here's an example of a user struggling with this problem: #363. They had a package (node-fetch) where require() is supposed to return a function. Bundlers that selected module over main broke code that used require() because it returned an object with a default property. So in node you'd have to do fetch = require('node-fetch') and with a bundler you'd have to do fetch = require('node-fetch').default. To fix this, esbuild now dynamically switches whether module or main is used based on how the file is imported (but only when the platform is set to browser, details here).

The specification for conditions is more complicated and rigid so module would always be chosen in your example. Enabling module by default will break code—specifically code that calls require and does not expect to get an ES module namespace exotic object back. I do not want esbuild's default behavior to be one that breaks code. My priorities are 1) make sure bundled code works and 2) optimize for size, in that order.

People might also want to start using exports maps within a minor version and adding an import condition might be a breaking change

Adding any exports map at all is a breaking change because it prevents importing anything that's not in that map, which people might be doing.

@Andarist
Copy link
Author

Here's an example of a user struggling with this problem: #363. They had a package (node-fetch) where require() is supposed to return a function. Bundlers that selected module over main broke code that used require() because it returned an object with a default property. So in node you'd have to do fetch = require('node-fetch') and with a bundler you'd have to do fetch = require('node-fetch').default. To fix this, esbuild now dynamically switches whether module or main is used based on how the file is imported (but only when the platform is set to browser, details here).

I know about this danger... but I somewhat strongly believe that this should "just" be fixed on that package's side. Trying to use the so-called "auto" exports (instead of named) when transpiling down to CJS is just a big source of pain for users and it has been like that for uears. It's almost guaranteed to break one way or another and, like you mentioned, it already breaks in some bundlers (like webpack). For that reason I've abandoned this pattern years ago - it's not worth the .

While there are different tradeoffs here to any potential solution and we could even attempt to disagree with the ones chosen by webpack... the status quo is that webpack is super popular and a package like this won't work with webpack. So while you have fixed~ this for esbuild users I think the problem is still in that package.

I've also re-checked with Rollup to see how this is handled there and it seems that they introduce an interop runtime helper that allows for both to be used at runtime (require('lib') and require('lib').default) in this particular case. They introduce a different tradeoff though: require('lib') !== require('lib').default.

Enabling module by default will break code—specifically code that calls require and does not expect to get an ES module namespace exotic object back.

I agree with that to some extent - but as mentioned - I would consider this to be a problem at the library's side because its structure makes it incompatible with other very popular tools.


At the end of the day, you will do whatever is good for your project in your eyes. I and respect that. I've just wanted to raise this as a potential problem because I've found out that esbuild is not adding this condition by default whereas other popular bundlers are. This makes my life even harder, as the matrix of potential configurations has increased because of that and figuring out how to satisfy different tools is/was already an almost impossible job.

@evanw
Copy link
Owner

evanw commented Jul 31, 2022

Ok. I will try to do the same thing for conditions that I do with mainFields then. Specifically: If you use import then esbuild will resolve twice, once with module and once without module. Bundling will proceed with the path with module, but if the path without module ever ends up being used, then esbuild will switch to exclusively using that instead.

This way if your package is specified with module and default like in your original post, using both import "pkg" and require("pkg") with your package means esbuild will pick just default (and you'll get correct default export behavior but no tree shaking) but using only import "pkg" with your package means esbuild will pick just module (and you'll get tree shaking). This is because import "pkg" will resolve to { primary: "<module path>", fallback: "<non-module path>" } while require("pkg") will resolve to { primary: "<non-module path>" }.

This will only happen if you don't specify your own conditions value and if the current platform is browser. If you do specify conditions, then esbuild will disable this behavior regarding module and just follow whatever conditions you specified.

I know about this danger... but I somewhat strongly believe that this should "just" be fixed on that package's side. Trying to use the so-called "auto" exports (instead of named) when transpiling down to CJS is just a big source of pain for users and it has been like that for uears. It's almost guaranteed to break one way or another and, like you mentioned, it already breaks in some bundlers (like webpack). For that reason I've abandoned this pattern years ago - it's not worth the .

I agree that the default export should basically never be used. The community has made a mess of it and using it practically guarantees that someone will run into compatibility problems with it at some point. But people use it all over the place and they aren't going to stop, so esbuild has to support it. That ship has sailed.

@evanw evanw closed this as completed in 5e918f4 Jul 31, 2022
@evanw
Copy link
Owner

evanw commented Jul 31, 2022

I tried implementing this as described above. However, I'm having second thoughts about this. This could still introduce the dual-package hazard for package subpaths. The problem is that the decision of "module or not" is made at each subpath. That's not a problem for "main fields" because they are only about the entry point, not about subpaths. So I think perhaps I should revert this for now.

@guybedford
Copy link
Contributor

I was under the impression previously that Webpack used the "module" condition to avoid the dual package hazard by explicitly matching that condition for both require and import since Webpack is an environment with the ability to require ES modules. Therefore I'm not sure I understand the need for ever not matching it, although in cases where it does cause issues being able to opt-out of it seems useful as a global configuration. Let me know if I've got the above incorrect though.

evanw added a commit that referenced this issue Jul 31, 2022
@evanw evanw reopened this Jul 31, 2022
@Andarist
Copy link
Author

Andarist commented Aug 1, 2022

This way if your package is specified with module and default like in your original post, using both import "pkg" and require("pkg") with your package means esbuild will pick just default (and you'll get correct default export behavior but no tree shaking) but using only import "pkg" with your package means esbuild will pick just module (and you'll get tree shaking). This is because import "pkg" will resolve to { primary: "", fallback: "" } while require("pkg") will resolve to { primary: "" }.

I think this is a little bit complicated and that it still might cause ambiguities between consuming projects but I'm OK with this, better to have this than nothing from my PoV. What, IMHO, has to be preserved is the lack of dual package hazard as that's the whole reason to have the module condition in the first place.

I agree that the default export should basically never be used. The community has made a mess of it and using it practically guarantees that someone will run into compatibility problems with it at some point. But people use it all over the place and they aren't going to stop, so esbuild has to support it. That ship has sailed.

"auto" exports were somewhat popular with package.json#{main,module}. I wonder if they are still popular with the module condition. I would like to think that by now people have learned their lesson and that they don't try to provide a different exports shape between their conditions etc. It's hard to really know this but I think that there is a real chance here that "auto" is not popular in packages with exports. Please don't quote me on that though 🤣

I tried implementing this as described above. However, I'm having second thoughts about this. This could still introduce the dual-package hazard for package subpaths. The problem is that the decision of "module or not" is made at each subpath. That's not a problem for "main fields" because they are only about the entry point, not about subpaths. So I think perhaps I should revert this for now.

I guess that if you want to have this logic for using this condition conditionally, as described above, then the solution here would be to have this decision applied per package root?

Therefore I'm not sure I understand the need for ever not matching it, although in cases where it does cause issues being able to opt-out of it seems useful as a global configuration. Let me know if I've got the above incorrect though.

From what I understand, @evanw is worried about require returning the "auto"/flattened shape for packages with just a default export and thus having a different shape than their ESM equivalent which in turn can result in some runtime errors if what gets returned from require change its meaning for the consumer.

@Andarist
Copy link
Author

Andarist commented Dec 7, 2022

@evanw thanks ❤️

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

No branches or pull requests

3 participants