-
Notifications
You must be signed in to change notification settings - Fork 30k
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
module: deprecate "main" index and extension lookups #36918
Conversation
@nodejs/modules |
Should this perhaps be Semver-Major? |
If we did want to ship this it might make sense to try to ship it soon so we don't break unmaintained ESM packages. What's the usual policy on a log-only deprecation? |
Runtime deprecations are semver-major according to https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#deprecations |
Thanks @aduh95 I've put this deprecation beind |
What's the benefit of doing this? What about |
@ljharb it means that a package like: {
"main": "dir"
} where the main is eg |
You should probably document the deprecation in |
@guybedford right, i understand the consequences. why, though? |
Because Assuming that’s the reasoning (?) I guess I can get behind that, but it’s a big change that should probably have more than a few eyes on it. It might be something we should write a blog post about, encouraging the adoption of |
@GeoffreyBooth ok, but why not do so for any package that declares an explicit |
Sure, we could do that for any Also arguably we might not want to deprecate |
I'm -0 on this since it makes "type" have a linting like effect rather than just being configuration of the file types. There are minutiae we could argue about this, but if we ever do overload |
IMO, {
"type": "module",
"main": "./lib/index.js"
} But the following for the same directory structure is deprecated: {
"type": "module",
"main": "./lib/index"
} {
"type": "module",
"main": "./lib"
} In other words, I prefer alternative option 2. |
@GeoffreyBooth a package is not a "commonJS package" or an "ESM package" except by virtue of what files are contained within it. A package with type "module" is no more an ESM package than one with no type at all, assuming both provide (or do not provide) ESM modules, and there should be no such implication by any use of the "type" field. |
I don’t see us expanding As for an extensions map, the functionality that that would unlock is possible with loaders today and will become easier as that API continues to evolve (#36396). Loaders wouldn’t help published packages, but I’m not sure that mapping extensions within dependencies is a use case we need to support. |
This PR as written effectively deprecates the
@bmeck in terms of more blanket proposals, we could eg outright deprecate the main in packages and for example warn when executing a local package without an exports field. The difficulty with a blanket warning for non-exports is that's even more agressive than what this PR is doing. The less agressive options are likely to refine the semantics of main. Alternatively we can just do nothing here and let |
I guess the big question here is do we want to "force" encapsulation of packages, or allow using the "main" as a way to continue to optionally choose to use encapsulation or not. I'm tending towards thinking making encapsulation optional might be better after all? |
That matches my understanding of the original intention - opt-in encapsulation, not forced. |
To try and flesh out some of the options we have here then:
Any opinions? At least picking a coherent story going forward seems like it might make sense at this point. |
I would prefer this at the least since if we add extension support over time the resolved file may change. Deprecation would allow some future proofing and get back some space avoiding compatibility concerns. The implicit |
Here's a alternative which is options 1 and 3 combined:
|
Would this mean self imports of the current package wouldn't use it / would error? |
@bmeck I think that's already the case (self-reference only sees By |
This adds a new deprecation warning when importing a main that resolves to an ES module that relies on the "index" or extension searching "main" resolution semantics.
In the next major this can become a runtime error, while for now it is behind
--pending-deprecation
only.This is an update to the previous iteration which allows "main" and "exports" to continue to coexist without encapsulation being enforced.
The two warnings shown are for extensions not present:
and for no main field present:
The warning is shown for third party packages and local packages equally.
This approach was taken after discussion that pushing encapsulation or deprecating the main would be too strong of a move to make.