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

esm: remove unused function argument #35072

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 6, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 6, 2020
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.

I believe this used to be to provide the "importer" information in the error message on failure, which is important in esm because we don't have a require stack.

I just checked this and can verify that for package.json with invalid syntax that importing in that directory gives the invalid syntax error without the name context of the parent module.

I would prefer if we could rather refactor getPackageConfig to support the " imported from ${base}" error context here rather than remove this context.

@Trott Trott marked this pull request as draft September 6, 2020 00:15
@Trott
Copy link
Member Author

Trott commented Sep 7, 2020

I just checked this and can verify that for package.json with invalid syntax that importing in that directory gives the invalid syntax error without the name context of the parent module.

Are you referring to ERR_INVALID_PACKAGE_CONFIG or something else? I'm not sure if I'm following what you're saying.

@Trott
Copy link
Member Author

Trott commented Sep 7, 2020

(And if this is one of those things that would be easier/faster to just do yourself than explain to me, don't let me stop you.)

@guybedford
Copy link
Contributor

guybedford commented Sep 9, 2020

I just had a deeper look at this and there are two main cases for the ERR_INVALID_PACKAGE_CONFIG error:

  1. import 'pkg' doing a node_modules lookup for the package will read the package.json, then throw this error if it has invalid syntax
  2. import './path/to/module.js' will do a package.json lookup for the module format of the JS file and will throw this error if the package.json file has invalid syntax

For case (1), useful context is the parent importer loading the package.
For case (2), useful context is the module that was loaded that caused the package.json to be checked.

Both of these are actually slightly different messages though. I've gone ahead and put together a nicer context for these messages in #35117.

@Trott Trott closed this Sep 9, 2020
@Trott Trott deleted the esm-arg branch April 14, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants