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: improve error for invalid package targets #32052

Closed
wants to merge 9 commits into from
Closed

module: improve error for invalid package targets #32052

wants to merge 9 commits into from

Conversation

MylesBorins
Copy link
Contributor

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-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Mar 2, 2020
jkrems
jkrems reviewed Mar 2, 2020
View reviewed changes
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.

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

lib/internal/errors.js Outdated Show resolved Hide resolved
test/es-module/test-esm-exports.mjs Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Mar 2, 2020

LGTM pending jkrem's comment

@MylesBorins
Copy link
Contributor Author

Good catch @jkrems PTAL

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

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

lib/internal/errors.js Outdated Show resolved Hide resolved
@MylesBorins MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 10, 2020
@MylesBorins MylesBorins 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
Contributor Author

@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

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

ljharb
ljharb reviewed Apr 17, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

guybedford
guybedford reviewed Apr 17, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
guybedford
guybedford approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@guybedford guybedford 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
lib/internal/errors.js Outdated Show resolved Hide resolved
MylesBorins and others added 2 commits April 20, 2020 22:07
@MylesBorins @guybedford
Update lib/internal/errors.js
bb4a755 
Co-Authored-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins @guybedford
Update lib/internal/errors.js
5ca9689 
Co-Authored-By: Guy Bedford <guybedford@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

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

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

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

lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
@MylesBorins @GeoffreyBooth
Update lib/internal/errors.js
c5f08ce 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
MylesBorins and others added 3 commits April 21, 2020 15:38
@MylesBorins @GeoffreyBooth
Update lib/internal/errors.js
51de45b 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@MylesBorins @GeoffreyBooth
Update lib/internal/errors.js
8e04ec1 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@MylesBorins @GeoffreyBooth
Update lib/internal/errors.js
ef6c799 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@nodejs-github-bot

This comment has been minimized.

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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: #32052
Fixes: #32034
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Myles Borins <myles.borins@gmail.com>
@MylesBorins
Copy link
Contributor Author

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: #32052
Fixes: #32034
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Myles Borins <myles.borins@gmail.com>
@BethGriggs BethGriggs 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: #32052
Fixes: #32034
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Myles Borins <myles.borins@gmail.com>
@BridgeAR BridgeAR 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: #32052
Fixes: #32034
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Myles Borins <myles.borins@gmail.com>
@targos targos 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: #32052
Fixes: #32034
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DerekNonGeneric DerekNonGeneric DerekNonGeneric left review comments

@guybedford guybedford guybedford approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@ljharb ljharb ljharb left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

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

package.exports require "./" in front of path or resolver fails
8 participants
@MylesBorins @ljharb @nodejs-github-bot @GeoffreyBooth @jkrems @guybedford @BridgeAR @DerekNonGeneric