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

Closed
wants to merge 9 commits into from

Conversation

MylesBorins
Copy link
Member

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

Good catch @jkrems PTAL

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
3 tasks
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
Member 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

lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

lib/internal/errors.js Outdated Show resolved Hide resolved
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.

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

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
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
MylesBorins and others added 3 commits April 21, 2020 15:38
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

MylesBorins added a commit that referenced this pull request Apr 22, 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

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

landed in 09a50d3

BethGriggs pushed a commit that referenced this pull request Apr 27, 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

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
BridgeAR pushed a commit that referenced this pull request Apr 28, 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

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
targos pushed a commit that referenced this pull request Apr 30, 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

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 pushed a commit that referenced this pull request May 13, 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

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
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
Development

Successfully merging this pull request may close these issues.

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