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

npm package inconsistently resolved when imported from v1 vs. v2 addon #556

Open
simonihmig opened this issue Dec 19, 2022 · 5 comments
Open

Comments

@simonihmig
Copy link
Contributor

We are importing a library (three.js) both from a v1 as well as from a v2 addon.

For the v1 addon, this causes an AMD shim module to be generated, which pulls the CJS verison of it via webpack:

d('three', [], function() { return __webpack_require__(\"../../../../node_modules/three/build/three.cjs\"); });

This is then used as a dependency of any of the v1 addon's modules, like

define("some-module",["exports","three"],function() {...})

Works fine! But now the v2 addon also imports it, but this time it gets the ESM version (.../build/three.module.js). You see that in the eval() code that is generated (in dev) for the v2 module.

So we end up with two versions of the same library in our bundle. I was able to workaround this, by forcing eai to use the ESM version everywhere using an alias entry in its config. But I wonder why that is, and if we can fix this upstream?

For the record, this is how three defines its exports in its package.json

@ef4
Copy link
Collaborator

ef4 commented Dec 19, 2022

Thanks for the clear report. Two concerns:

Forcing eager evaluation

We could generate the shims as ES imports instead of require, and that would presumably fix this. But the tradeoff is, it would force us to move their evaluation to become eager. Right now we hand webpack something like:

window.define("some-module", function() { return require("some-module") });

And webpack sees the require() as a CommonJS require and handles it, whereas the window.define is left for the Ember AMD loader at runtime. This wires the two worlds together.

We cannot change that to this because there's no place to await the promise:

window.define("some-module", function() { return import("some-module") });

So instead we would need to make it:

import * as SomeModule from "some-module";
window.define("some-module", function() { return SomeModule });

Which is correct, but moves the evaluation time of some-module so that it runs eagerly, even if the app never actually enters a codepath that needs it.

Maybe this is worth it and not a big deal? It's certainly true that over the long run, any truly ES-module-based tooling is going to need to work this way. So long as Ember does synchronous require() to find things, we can't lazily evaluate their modules.

Duplication either way

Even if we made the above change, you could still get duplication if one of your dependencies is written in CommonJS and does require() for the library, while you're doing import for it.

This seems to be the intended way for package.json exports to work. I think in these cases, duplication is a feature and not a bug, because each consumer is getting the copy they "asked for".

Ultimately I think what we're saying here is that we need to eventually drop support for CommonJS and have a purely modules-based build. But that is more of an Embroider feature than an ember-auto-import feature. My main goal here is to find the least painful migration path that lets people use ember-auto-import as a bridge to Embroider.

@ef4
Copy link
Collaborator

ef4 commented Dec 19, 2022

Addendum: the parse of the modules is already eager, because they're necessarily in the bundle because they need to be synchronously available. The thing we're talking about changing is just the eval of the modules. Which is probably not that expensive...

(To avoid the eager parse, people need to use import() directly in their code instead of static imports)

@simonihmig
Copy link
Contributor Author

Thanks for the thorough explanation, all of this makes sense!

I actually wondered if we could still prefer the ESM version of a package (i.e. the module field) when available, even when require()ed? After all that's what I'm doing with my alias. But I guess that could break things in some cases due to different module semantics (e.g. default in ESM)?

@ef4
Copy link
Collaborator

ef4 commented Dec 20, 2022

It’s probably possible. The CJS/ESM interop is already smoothing over default exports. Just need to figure out the right webpack config that would tell it to ignore the require stuff in package json exports.

@simonihmig
Copy link
Contributor Author

simonihmig commented Oct 18, 2024

So after working on #641 and the discussions that followed, I tried the following:

Use global resolve.conditionNames

Adding this to webpack config enforces using the import type of package.json#exports and ignoring require:

    resolve: {
      conditionNames: ['import', 'browser', 'default'],
    },

Although that makes me slightly worried that this is now applied globally, not just for the known to be bad cases. But It proved to be working in our large codebase.

Just need to figure out the right webpack config that would tell it to ignore the require stuff in package json exports.

We could apply that to eai's default config I guess, but I am not sure if this couldn't accidentally break things, e.g when packages have badly declared exports, which happened to work before!? 🤔

Use per module modules.rules[].resolve.conditionNames

Webpack docs say you can apply that setting also to just specific module rules, however I wasn't able to get this to work. I also don't quite understand how this could work conceptionally, since the test or resource conditions of a module rule are referring to an already resolved file on disk (docs), so how would webpack apply new resolve rules on an already resolved resource? Other discussions also seem to suggest this doesn't work...

Off-topic: did I mention webpack docs are not great? 😩

Use resolve.alias

Instead of using an alias with an absolute file path (which brought up #616) you can try to alias with a relative one. But that seems to make webpack still resolve through package.json#exports, which might work or might not...

For example:

alias: {
  sinon: 'sinon/pkg/sinon-esm',
  '@faker-js/faker/locale/en':  '@faker-js/faker/dist/locale/en.js',
},

The first will work, since sinon's exports allow importing ./pkg/sinon-esm (or any other path really) here. But faker will not, because the explicit ./dist parts isn't matched anywhere here.

custom webpack plugin

You could probably write a custom plugin, that resolves to esm builds only for specific packages. But haven't tried that yet, as writing webpack plugins isn't fun...

cc @ef4

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

2 participants