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: expose getPackageScopeConfig & add findNearestPackageJSON #54992

Closed

Conversation

JakobJingleheimer
Copy link
Contributor

This draft is not ready for review. Opening it in its current state for visibility.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. typings labels Sep 18, 2024
@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 18, 2024
@@ -64,6 +71,7 @@ function deserializePackageJSON(path, contents) {
ObjectDefineProperty(this, 'exports', { __proto__: null, value });
return this.exports;
},
...(everything && contents[6]),
Copy link
Member

Choose a reason for hiding this comment

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

Can we be explicit? Depending on the package.json this can overwrite existing values in this object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can't be explicit because we don't know what the keys are—that's the point of this feature ;) also, we don't need to worry about overwriting because this is a rest spread.

src/node_modules.cc Outdated Show resolved Hide resolved
src/node_modules.cc Show resolved Hide resolved
@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented Sep 29, 2024

I'm planning to adjust GetPackageJSON to optionally include all fields from the package.json. I think for unrecognised fields, I need to generically handle (parse) each kind of valid json value. Does that functionality already exist in c++, or do I need detect each type and handle the parsing specifically?

GetPackageJSON currently has a loop with a bunch of if (key = …), else if (key = …), else if (key = …). If I need to handle each kind of value, I'm thinking to extract those to some kind of hash-map:

  • PJSONParsers['exports']
  • PJSONParsers['imports']
  • PJSONParsers['name']
  • PJSONParsers['script']
  • PJSONParsers['type']
  • PJSONParsers['default'] ← handles unrecognised fields (which can have any valid json value, and calls various "nonspecific*" methods on PJSONParsers)
  • PJSONParsers['nonspecificString']
  • PJSONParsers['nonspecificMap'] ← will call other "nonspecific*" methods

Side-note: I haven't worked in c++ in ~20 years, so I can't remember if there is potentially a performance implication here. I'm thinking extracting those avoids re-constructed on each loop (but that may be offset by the cost of function call(s)).

@JakobJingleheimer
Copy link
Contributor Author

Splitting this into atomic PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. typings wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants