Skip to content

module: CJS / ESM resolver sharing and refactoring#34744

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.

Sign up for GitHub

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

Jump to bottom
Closed
guybedford wants to merge 2 commits into from
Closed

module: CJS / ESM resolver sharing and refactoring #34744

guybedford wants to merge 2 commits into from

Conversation

@guybedford
Copy link
Contributor

@guybedfordguybedford commented Aug 12, 2020•
edited
Loading

This extracts just the resolver refactoring component from #34718.

I hope that the negative diff speaks for itself at least!

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-botnodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 12, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2020•
edited by DerekNonGeneric
Loading

Review requested:

  • @nodejs/modules

@guybedfordguybedford changed the title module: exports patterns and resolver refactoringmodule: CJS / ESM resolver sharing and refactoringAug 12, 2020
@guybedfordguybedford mentioned this pull request Aug 12, 2020
Closed
4 tasks
@ljharb
Copy link
Member

ljharb commented Aug 12, 2020

Wait, so now you can map your exports to things in node_modules?? Can you help me understand why that's a good idea?

@guybedford
Copy link
ContributorAuthor

guybedford commented Aug 12, 2020

Sure, I can add it back, it's just that the spec was actually incorrect since it was only checking the package.json target path, when we in fact need to check both the target path and the user subpath. We give separate errors for these two cases because the one is package author issue while the other is a package consumer issue. This error separation doesn't apply as easily for patterns though so those errors may need to be rethought. I refactored it out because it seems overly strict and unnecessary but I can add it back.

@ljharb
Copy link
Member

ljharb commented Aug 12, 2020

I think it's very critical that "exports" only be possible to use to remap an entry specifier to "a file within the package that has exports".

@guybedford
Copy link
ContributorAuthor

guybedford commented Aug 12, 2020

@ljharb I've added back the node_modules restriction identically to previously with a refactoring to make it part of the new segment check approach.

ljharb
ljharb reviewed Aug 12, 2020
View reviewed changes
@guybedford
Copy link
ContributorAuthor

guybedford commented Aug 12, 2020

I would like to land this PR soon. Any further feedback welcome.

@guybedford
Copy link
ContributorAuthor

guybedford commented Aug 12, 2020

@nodejs/modules-active-members for any further review

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32750/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32751/

guybedford added a commit that referenced this pull request Aug 13, 2020
@guybedford
module: share CJS/ESM resolver fns, refactoring
f8976a7
PR-URL: #34744 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: James M Snell <[email protected]>
@guybedford
Copy link
ContributorAuthor

guybedford commented Aug 13, 2020

Landed in f8976a7.

addaleax added a commit to addaleax/node that referenced this pull request Aug 13, 2020
@addaleax
test: fix message/esm_loader_not_found_cjs_hint_bare
f53b806
Broken locally for me since f8976a7. Refs: nodejs#34744
@addaleaxaddaleax mentioned this pull request Aug 13, 2020
Closed
3 tasks
@guybedfordguybedford mentioned this pull request Aug 15, 2020
Closed
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
@guybedford@MylesBorins
module: share CJS/ESM resolver fns, refactoring
2f7cfe5
PR-URL: #34744 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: James M Snell <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@guybedford@BethGriggs
module: share CJS/ESM resolver fns, refactoring
698cae7
PR-URL: #34744 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: James M Snell <[email protected]>
guybedford added a commit to guybedford/node that referenced this pull request Sep 28, 2020
@guybedford
module: share CJS/ESM resolver fns, refactoring
7e20df2
PR-URL: nodejs#34744 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford@codebytere
module: share CJS/ESM resolver fns, refactoring
cecc193
PR-URL: #34744 Backport-PR-URL: #35385 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: James M Snell <[email protected]>
@codebyterecodebytere mentioned this pull request Oct 4, 2020
Merged
@cyyynthiacyyynthia mentioned this pull request Apr 7, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments

@jasnelljasnelljasnell approved these changes

@hybristhybristhybrist approved these changes

Assignees

No one assigned

Labels

errorsIssues and PRs related to JavaScript errors originated in Node.js core.esmIssues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@guybedford@nodejs-github-bot@ljharb@jasnell@hybrist@addaleax