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: self resolve bug fixes and esm ordering #31009

Closed
wants to merge 5 commits into from

Conversation

guybedford
Copy link
Contributor

This resolves the self-resolve bugs #30633 and #30602. In addition the ESM implementation of self-resolve is slightly adjusted to apply before the node_modules lookup for better encapsulation, while the CJS implementation is left after the node_modules lookup for backwards compatibility.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Dec 17, 2019
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

doc/api/esm.md Show resolved Hide resolved
test/es-module/test-esm-exports.mjs Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor

I'm not sure that we should change the resolve order between ESM + CJS. This could result in unexpected behavior.

@guybedford
Copy link
Contributor Author

guybedford commented Dec 17, 2019

@MylesBorins the alternatives here are:

  1. Allow self-resolve to always be intercepted by node_modules lookups in ES modules. My concern with this is that self-resolve is no longer reliable but dependent on the surrounding node_modules structures, which doesn't provide the same absolute guarantee of self resolution not having to even check the file system.
  2. Return to the great sigil debate of not using the package name string, since we have to support the node_modules lookups in the CommonJS implementation for the package name for backwards compatibility in existing CommonJS workflows.

I find this is enough of an edge case that the compat doesn't concern me personally, while the full encapsulation going forward I do think is important. Happy either way here though.

@weswigham
Copy link

weswigham commented Dec 17, 2019

This could result in unexpected behavior.

It'll behave strangely any time a package gets installed in its own node_modules folder (as the esm resolver will resolve to the local copy, while the cjs resolver will resolve to the node_modules copy). This used to happen when developing typescript, as typescript had a devDependency of tslint which had a dependency of typescript (which npm then flattens), so is a situation that happens in the real world. They probably shouldn't diverge.

@weswigham
Copy link

weswigham commented Dec 17, 2019

which doesn't provide the same absolute guarantee of self resolution not having to even check the file system.

It needs to check the fs to look for the enclosing package.jsons already - the node_modules folder state isn't that much more, once you're already hitting disk.

W.R.T. the sigil stuff, I, at least, forsee writing a

{
  "name": "~",
  "type": "module",
  "private": true
}

package.json file in my src folders, under my primary package.json. So you could say that the current implementation just allows choice of sigil, if you're so inclined. So if people are taking issue with the "meaning" of a sigil, I'm not sure the argument holds once this kind of "custom sigil" is possible. it may be better to just pick one, any one, just to save work in both resolvers.

@guybedford
Copy link
Contributor Author

@weswigham it will check node_modules/[name] folders all the way to the root of the file system, is the issue.

When applying tooling for eg import maps generation, a root-level node_modules install affecting self-resolution isn't ideal.

@weswigham
Copy link

it will check node_modules/[name] folders all the way to the root of the file system, is the issue.

Are the discovered folder contents cached, like how package.json results are cached? If so, it shouldn't be too bad, since resolution will already be forced to generate that list for literally any other non-relative specifier.

@jkrems
Copy link
Contributor

jkrems commented Dec 17, 2019

Return to the great sigil debate of not using the package name string, since we have to support the node_modules lookups in the CommonJS implementation for the package name for backwards compatibility in existing CommonJS workflows.

Using a sigil does unfortunately not really change this: Any sigil would be a valid sub-directory of node_modules and we're back to the same order concern for require. Afaik we haven't found any syntax that is backwards compatible with existing CJS resolution without checking node_modules first. The only solution would be an explicit opt-in like a useSelfReference in package.json which would work for both name and sigil.

@ljharb ljharb mentioned this pull request Dec 17, 2019
2 tasks
@weswigham
Copy link

weswigham commented Dec 17, 2019

Afaik we haven't found any syntax that is both backwards compatible with existing CJS resolution without checking node_modules first.

I think the idea behind picking a sigil is that you can make the cjs loader pick the sigil first, and it's probably not a really breaking change, since you generally can't use sigils as a package name (especially if they can't be package names on npm); meanwhile if using the package's actual name, if you change the behavior, you change a scenario that actually crops up in real installations.

@MylesBorins
Copy link
Contributor

Could we potentially tease the ordering change out of this PR and land the fixes? Or are they entangled?

@guybedford
Copy link
Contributor Author

guybedford commented Dec 19, 2019

Further to the discussion from the meeting today I've gone ahead and pushed the following change:

  • If the package scope has an "exports" property, then self resolution applies before any further package resolution lookup in node_modules
  • If the package scope does not have an "exports" property, then the self resolution always only applies after trying the node_modules package resolution. self resolution does not apply.
  • The implementation of the above is identical between CJS and ESM, bringing them back to parity.

The approach for this is basically to inject the self resolve method twice - once before and once after package resolution, with a flag to indicate if it must check for exports.

@jkrems does that alleviate your original concerns here? Would appreciate re-review when you can.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed out-of-band: LGTM in general although I'd like to see the 2nd attempt removed. Having a safe way of using this feature (set exports) and a default way that isn't as safe (post-node_modules lookup) seems dangerous. Thanks for working through this!

@guybedford
Copy link
Contributor Author

In line with the suggestion by @jkrems I've made self-resolution always-opt-in on "exports" being present for both.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think there's some traces left in the algorithm of the previous variant.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Dec 19, 2019

Wait, does this mean that self-export only works inside a package using "exports"?

@guybedford
Copy link
Contributor Author

Wait, does this mean that self-export only works inside a package using "exports"?

As added in ff11612, yes. The first iteration before this commit was a bit messier, but allowed it without exports too. The diff is in #31009 (comment) to see what I mean.

Since "exports" is the new main which also offers encapsulation this approach seems to make the most sense. I'd advise checking the meeting notes on the discussion to avoid rehashing too much here, but happy to discuss further.

@MylesBorins MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 23, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Dec 29, 2019
@guybedford
Copy link
Contributor Author

Landed in 8a96d05.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants