nodejs / modules

Node.js Modules Team
MIT License
413 stars 42 forks source link

Clarification for bundlers re "import" / "require" matching #556

Closed guybedford closed 3 years ago

guybedford commented 4 years ago

It came up in discussion with RollupJS and Webpack implementations that the idea of "import" and "require" being not only mutually exclusive but also exhaustive may rule out some cases like asset resolutions where they want to use the same resolver to eg resolve image stylesheet imports from within a stylesheet.

In Webpack currently, @import in sass will use neither the "require" nor the "import" condition.

I was wondering how we want to clarify the Node.js guidance as we define these terms - should other resolvers always use one of these or do we relax the guidance to permit them both to be disabled, so long as they remain mutually exclusive?

Another point on this topic is what import.meta.resolve should use. Currently it will by default use the environment conditions and "import". It could be an option to relax that as well to not use the "import" condition since it is an abstract resolution operation.

I don't want to get into allowing custom condition arguments to this resolver spec ideally though, although perhaps that is where this goes? Either way does "import" as the initial starting point make sense?

jkrems commented 4 years ago

I was wondering how we want to clarify the Node.js guidance as we define these terms - should other resolvers always use one of these or do we relax the guidance to permit them both to be disabled, so long as they remain mutually exclusive?

It sounds like we don't really have a choice here - they won't be exhaustive in some cases. And even if node.js doesn't care, there's module system outside of require and import. And CSS @import is fairly established ("fairly" because it's not always used in source in the same way that it's used in browsers).

In Webpack currently, @import in sass will use neither the "require" nor the "import" condition.

That sounds consistent since @import isn't using the (JS) import system, at least not when talking about the "real" @import in browsers.

It could be an option to relax that as well to not use the "import" condition since it is an abstract resolution operation. [...] I don't want to get into allowing custom condition arguments to this resolver spec ideally though, although perhaps that is where this goes?

I think if it's attached to import.meta, it would be surprising if it doesn't resolve like import() (just like require.resolve should resolve like require()).

I would expect a more generic resolution function that also accepts conditions to not be on import.meta but to be imported explicitly from somewhere. I think the base url argument already stretches the "is this still a contextual API?" question. The more general it becomes, the less convinced I'd be that it should still live on import.meta.

ljharb commented 4 years ago

Seems like a sass file would specify a "sass" condition, for example - why would they want to use import or require, when neither one is actually importable or requireable?

jkrems commented 4 years ago

Seems like a sass file would specify a "sass" condition, for example - why would they want to use import or require, when neither one is actually importable or requireable?

Agreed! But right now our docs suggest that require and import are exhaustive. Sounds like everybody so far agrees that we should remove that claim as a general rule and at most call out that it applies by default in node.

bmeck commented 4 years ago

I'd be fine removing the wording on it being exhaustive entirely.

ljharb commented 4 years ago

I think it is exhaustive, for things node supports. Bundler-only things (like sass/css/images) aren't something node supports.

guybedford commented 4 years ago

Thanks all for the input here, agreed relaxing sounds sensible. I've posted https://github.com/nodejs/node/pull/35311. Further comments / suggestions there welcome.

bmeck commented 3 years ago

the clarification has landed