Skip to content

module: improve error for invalid package targets#32052

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
MylesBorins wants to merge 9 commits into from
Closed

module: improve error for invalid package targets #32052

MylesBorins wants to merge 9 commits into from

Conversation

@MylesBorins
Copy link
Contributor

@MylesBorinsMylesBorins commented Mar 2, 2020

For targets that are strings that do not start with ./ or / the
error will now have additional information about what the programming
error is.

Closes: #32034

PTAL @nodejs/modules

@nodejs-github-botnodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Mar 2, 2020
hybrist
hybrist reviewed Mar 2, 2020
View reviewed changes
Copy link
Contributor

@hybristhybrist left a comment

Choose a reason for hiding this comment

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

Woo! I think the error message doesn't match what we currently allow for exports values but overall looks good!

@ljharb
Copy link
Member

ljharb commented Mar 2, 2020

LGTM pending jkrem's comment

@MylesBorins
Copy link
ContributorAuthor

MylesBorins commented Mar 2, 2020

Good catch @jkrems PTAL

guybedford
guybedford reviewed Mar 3, 2020
View reviewed changes
Copy link
Contributor

@guybedfordguybedford left a comment

Choose a reason for hiding this comment

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

Thanks for posting, this is huge for usability.

Would definitely be nice to see the tests covering the message if possible.

@MylesBorinsMylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 10, 2020
@MylesBorinsMylesBorins mentioned this pull request Mar 11, 2020
Closed
3 tasks
@MylesBorins
module: improve error for invalid package targets
974e6c0
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: nodejs#32034
@MylesBorins
Copy link
ContributorAuthor

MylesBorins commented Apr 17, 2020

@nodejs/modules I've updated based on all feedback. Should work for both ESM + CJS and has a test that checks the error message.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 17, 2020

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

ljharb
ljharb reviewed Apr 17, 2020
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 17, 2020

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

DerekNonGeneric
DerekNonGeneric reviewed Apr 17, 2020
View reviewed changes
DerekNonGeneric
DerekNonGeneric reviewed Apr 17, 2020
View reviewed changes
DerekNonGeneric
DerekNonGeneric reviewed Apr 17, 2020
View reviewed changes
guybedford
guybedford reviewed Apr 17, 2020
View reviewed changes
guybedford
guybedford approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@guybedfordguybedford left a comment

Choose a reason for hiding this comment

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

Amazing thanks for driving this one through.

guybedford
guybedford reviewed Apr 17, 2020
View reviewed changes
MylesBorinsand others added 2 commits April 20, 2020 22:07
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 21, 2020

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

GeoffreyBooth
GeoffreyBooth reviewed Apr 21, 2020
View reviewed changes
Copy link
Member

@GeoffreyBoothGeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Feel free to disregard if you disagree, I just feel like a semicolon here is better grammar than -.

@MylesBorins@GeoffreyBooth
Update lib/internal/errors.js
c5f08ce
Co-Authored-By: Geoffrey Booth <[email protected]>
MylesBorinsand others added 3 commits April 21, 2020 15:38
@MylesBorins@GeoffreyBooth
Update lib/internal/errors.js
51de45b
Co-Authored-By: Geoffrey Booth <[email protected]>
@MylesBorins@GeoffreyBooth
Update lib/internal/errors.js
8e04ec1
Co-Authored-By: Geoffrey Booth <[email protected]>
@MylesBorins@GeoffreyBooth
Update lib/internal/errors.js
ef6c799
Co-Authored-By: Geoffrey Booth <[email protected]>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 21, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 21, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 22, 2020

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

MylesBorins added a commit that referenced this pull request Apr 22, 2020
@MylesBorins
module: improve error for invalid package targets
09a50d3
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: #32034 PR-URL: #32052Fixes: #32034 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Signed-off-by: Myles Borins <[email protected]>
@MylesBorins
Copy link
ContributorAuthor

MylesBorins commented Apr 22, 2020

landed in 09a50d3

BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@MylesBorins@BethGriggs
module: improve error for invalid package targets
eaf841d
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: #32034 PR-URL: #32052Fixes: #32034 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Signed-off-by: Myles Borins <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@MylesBorins@BridgeAR
module: improve error for invalid package targets
e540d5c
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: #32034 PR-URL: #32052Fixes: #32034 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Signed-off-by: Myles Borins <[email protected]>
@BridgeARBridgeAR mentioned this pull request Apr 28, 2020
Merged
targos pushed a commit that referenced this pull request Apr 30, 2020
@MylesBorins@targos
module: improve error for invalid package targets
958ff15
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: #32034 PR-URL: #32052Fixes: #32034 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Signed-off-by: Myles Borins <[email protected]>
@targostargos mentioned this pull request May 2, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
@MylesBorins@targos
module: improve error for invalid package targets
3dc3772
For targets that are strings that do not start with `./` or `/` the error will now have additional information about what the programming error is. Closes: #32034 PR-URL: #32052Fixes: #32034 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Signed-off-by: Myles Borins <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments

@GeoffreyBoothGeoffreyBoothGeoffreyBooth approved these changes

@guybedfordguybedfordguybedford approved these changes

@hybristhybristhybrist approved these changes

@BridgeARBridgeARBridgeAR approved these changes

+1 more reviewer

@DerekNonGenericDerekNonGenericDerekNonGeneric left review comments

Reviewers whose approvals may not affect merge requirements

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.

package.exports require "./" in front of path or resolver fails

8 participants

@MylesBorins@ljharb@nodejs-github-bot@GeoffreyBooth@hybrist@guybedford@BridgeAR@DerekNonGeneric