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

guybedford commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

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

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 17, 2019

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

@guybedford

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

@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

This comment has been minimized.

Copy link

weswigham commented Dec 17, 2019

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

This comment has been minimized.

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 of 2 tasks complete
@weswigham

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 18, 2019

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

@guybedford

This comment has been minimized.

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 left a comment

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

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 19, 2019

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

@jkrems
jkrems approved these changes Dec 19, 2019
Copy link
Contributor

jkrems left a comment

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

This comment has been minimized.

Copy link
Member

ljharb commented Dec 19, 2019

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

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 19, 2019

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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

guybedford added a commit that referenced this pull request Dec 29, 2019
PR-URL: #31009
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 29, 2019

Landed in 8a96d05.

@guybedford guybedford closed this Dec 29, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
PR-URL: #31009
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins added a commit that referenced this pull request Jan 12, 2020
PR-URL: #31009
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford guybedford mentioned this pull request Jan 20, 2020
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.