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.

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

module: CJS / ESM resolver sharing and refactoring #34744

Closed
wants to merge 2 commits into from
Closed

module: CJS / ESM resolver sharing and refactoring #34744

wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

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

@guybedford guybedford changed the title module: exports patterns and resolver refactoring module: CJS / ESM resolver sharing and refactoring Aug 12, 2020
@guybedford guybedford 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
Contributor Author

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
Contributor Author

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

@guybedford
Copy link
Contributor Author

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

@guybedford
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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 <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in f8976a7.

@guybedford guybedford closed this Aug 13, 2020
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
@addaleax addaleax mentioned this pull request Aug 13, 2020
Closed
3 tasks
@guybedford guybedford 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 <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams 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 <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <jan.krems@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@jkrems jkrems jkrems approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues 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 @jkrems @addaleax