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

module: deprecate "main" index and extension lookups #36918

Closed

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 13, 2021

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:

Package /home/guybedford/Projects/node/test/fixtures/node_modules/default_index/ has a "main" field set to "index", excluding the full extension to the resolved file at "index.js", imported from /home/guybedford/Projects/node/test/fixtures/pkgexports.mjs.
 Automatic extension resolution of the "main" field is deprecated for ES modules.

and for no main field present:

No "main" or "exports" field defined in the package.json for /home/guybedford/Projects/node/test/fixtures/node_modules/no_exports/ resolving the main entry point "index.js", imported from /home/guybedford/Projects/node/test/fixtures/pkgexports.mjs.
Default "index" lookups for the main are deprecated for explicit definitions.

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.

@guybedford
Copy link
Contributor Author

@nodejs/modules

@MylesBorins
Copy link
Member

Should this perhaps be Semver-Major?

@guybedford
Copy link
Contributor Author

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?

@aduh95
Copy link
Contributor

aduh95 commented Jan 13, 2021

Runtime deprecations are semver-major according to https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#deprecations

@aduh95 aduh95 added notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 13, 2021
@guybedford
Copy link
Contributor Author

Thanks @aduh95 I've put this deprecation beind --pending-deprecation.

@guybedford guybedford removed notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 13, 2021
@ljharb
Copy link
Member

ljharb commented Jan 13, 2021

What's the benefit of doing this? What about "type": "commonjs" packages?

@guybedford
Copy link
Contributor Author

@ljharb it means that a package like:

{
  "main": "dir"
}

where the main is eg dir/index.js, would only work when not using "type": "module" - so we restrict the main index lookup to CommonJS modules.

@aduh95
Copy link
Contributor

aduh95 commented Jan 13, 2021

You should probably document the deprecation in packages.md as well.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2021

@guybedford right, i understand the consequences. why, though? type: module has no necessary correlation to "uses ESM", nor is that specific type something we're necessarily recommending (it's just an available option).

@GeoffreyBooth
Copy link
Member

why, though?

Because "exports" is better than "main"? Or put another way, "exports" is aware of the multiple module systems and can support distinguishing between them, whereas "main" can’t; and while that’s not strictly necessary for an ESM-only package, many packages will be dual-system for the next few years and therefore we want to push them to use "exports". It would be too disruptive to deprecate "main" entirely, so we’re taking the approach of deprecating it only for "type": "module" packages.

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 "exports".

@aduh95 aduh95 added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 13, 2021
@ljharb
Copy link
Member

ljharb commented Jan 13, 2021

@GeoffreyBooth ok, but why not do so for any package that declares an explicit type, including "type": "commonjs"? Why is type module special?

@GeoffreyBooth
Copy link
Member

@GeoffreyBooth ok, but why not do so for any package that declares an explicit type, including "type": "commonjs"? Why is type module special?

Sure, we could do that for any "type". It might be confusing though that this deprecation only appears when "type": "commonjs" is explicit rather than implied (by virtue of being the default).

Also arguably we might not want to deprecate "main" for CommonJS packages, as "exports" isn’t exactly fully backward-compatible since you can’t opt out of the encapsulation (I think?).

@bmeck
Copy link
Member

bmeck commented Jan 14, 2021

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 "type" to have objects on the RHS for things like custom format mappings this gets complicated. Also, while I do agree in theory "exports" is better than "main" I don't really see the need to simplify here. We would be breaking users so I would at least like to get some active returns over linting. If there was a semantic reason to cause an error that seems like a good reason to do this, but I don't see any clear why except that we can here. Things like static resolution are still possible even with the current behavior (albeit complex). I would be happy if we at least took a strong stance that we are trying to avoid the searching behavior there in this PR. I would also prefer we take a stance that if we do allow configuration of "type" to expand such as "wasm", file extension mappings, etc. on what we want to do with this behavior. If we have the presence of "type" always ban this type of index resolution (including in CJS packages) at least we can make a firm stance across the board here.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jan 14, 2021

IMO, type set to anything other than "commonjs" should deprecate the CommonJS‑style resolution of the main field, e.g. the following is valid:

{
	"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.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2021

@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.

@GeoffreyBooth
Copy link
Member

if we ever do overload "type"

I don’t see us expanding "type", at least not within the next semver major release of Node (or the next two releases even). When "type": "wasm" was discussed, it was for the purposes of defining how Node should treat extensionless files; but since then we’ve pretty much settled on always treating extensionless files as CommonJS, even inside a "type": "module" scope. (Hence the complaints in #32316.) As long as that doesn’t change, and I vaguely remember that there are strong reasons why it can’t (hence why we had to revert "type": "wasm") then extensions will always be required in ESM and therefore we don’t need configuration to tell Node how to handle a Wasm file (or any other non-JavaScript file).

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.

doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

This PR as written effectively deprecates the "main" field for Node.js. There are variations that could restrict what valid "main" fields are permitted, eg not defaulting to index.js and enforcing that main points to a valid path without extension searching.

"type": "module" works as the deprecation path here because .js files in main would always be CommonJS otherwise (and extension searching does not include .mjs). So while we don't capture the "main": "./x.mjs" case as being a deprecation in this PR, by effectively deprecating for "type": "module" it's the first runtime deprecation step to a full deprecation for "main" when using ES modules in Node.js.

@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 index.js remain as an ease of use vestige going forward. Note that index.mjs is not supported though so that there is a natural "type": "module" asymmetry already.

@guybedford
Copy link
Contributor Author

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?

@ljharb
Copy link
Member

ljharb commented Jan 14, 2021

That matches my understanding of the original intention - opt-in encapsulation, not forced.

@guybedford
Copy link
Contributor Author

To try and flesh out some of the options we have here then:

  1. Use this as an opportunity to enforce exports, deprecating main and non-encapsulated packages
  2. Deprecate the 'implicit' main index.js, and warn if no explicit "main" is provided
  3. Deprecate the extension searching for main, warning if extension searching on main is undertaken
  4. Keep main around (do nothing)
  5. Keep main around, but also support an index.mjs implicit main (possibly index.cjs as well?) so we don't have an asymmetrical implementation in future

Any opinions? At least picking a coherent story going forward seems like it might make sense at this point.

@bmeck
Copy link
Member

bmeck commented Jan 15, 2021

Deprecate the extension searching for main, warning if extension searching on main is undertaken

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 index.* seems also like it would need to be deprecated if this occurs though.

@aduh95
Copy link
Contributor

aduh95 commented Jan 15, 2021

Here's a alternative which is options 1 and 3 combined:

  • doc-deprecate "main" altogether – runtime deprecate only for local development.
  • document that package authors that don't want package encapsulation can either:
    • opt-out of encapsulation in their "exports" fields with "./*": "./*".
    • define neither "main" nor "exports" and rely on ./index.js default resolution – legacy behaviour
  • runtime deprecate resolving to an ES module using "main" or ./index.js default resolution. remove support for legacy resolution and extension searching for "type": "module" packages when using import.

@bmeck
Copy link
Member

bmeck commented Jan 15, 2021

@aduh95

doc-deprecate "main" altogether – runtime deprecate only for local development.

Would this mean self imports of the current package wouldn't use it / would error?

@aduh95
Copy link
Contributor

aduh95 commented Jan 15, 2021

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 "exports").

By runtime deprecate only for local development, I mean using require on directories that do not have package.json#exports would emit a warning (unless this is done by a module inside a node_modules folder) encouraging to use self-reference instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants