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: empty object fix in InternalModuleReadJSON for #30245 #30256

Closed

Conversation

guybedford
Copy link
Contributor

This fixes the bug posted in #30245, where having a package.json with no "type" field would not apply the CommonJS default.

The issue turned out to be that InternalModuleReadJSON applies a string filter to avoid parsing unnecessary package.json files, but the case of the filter failing and a file being not found were indistinguishable. (and that is why in that issue report, having a "repository" field with a "type" was enough to get it to work again!)

The implemented fix returns "{}" for when the filter finds no known package.json properties, to indicate it exists but has no properties we are interested in to avoid treating it the same as a not found package.json file. This way the default can still apply.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Nov 5, 2019
@guybedford
Copy link
Contributor Author

Thinking about this package.json read function a little more - it could be beneficial to write a custom package.json parser that applies the filtering during parsing itself. Since we scan the string now multiple times for each of the possible properties anyway it should actually be faster than the existing approach.

A C++ JSON parser that builds up JavaScript objects just for the "main" / "exports" / "type" fields we care about should be ideal for perf since the number of properties is always small.

@devsnek
Copy link
Member

devsnek commented Nov 5, 2019

Can you handle this on the JS side instead? wasting time parsing objects we know are already empty is not ideal.

@guybedford
Copy link
Contributor Author

@devsnek the saving of not having to parse the JSON source still applies - since we are just parsing JSON.parse("{}") for package.json files which are known not to have properties we need. This really is not a performance concern.

It would be slower performance to fix on the JS side because we would need to do a stat + a load of JSON instead of just trying to load the file.

@devsnek
Copy link
Member

devsnek commented Nov 5, 2019

@guybedford isn't it just checking if the function returns undefined?

@guybedford
Copy link
Contributor Author

The bug is that undefined is being returned both for a package.json file that does not exist and a package.json file that does exist but doesn't pass the filter. So we can't distinguish between not found files and existing files with no fields we care about.

@devsnek
Copy link
Member

devsnek commented Nov 5, 2019

That makes sense. If we don't have benchmarks for this already it might be worth adding them. In the past a lot of work went into optimizing package.json loading for cjs, and I'd imagine its slowed down a lot since then with all the work to esm.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@guybedford
Copy link
Contributor Author

Would be great to get another approval here to land this bug fix soon.

@guybedford
Copy link
Contributor Author

//cc @jkrems

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2019

@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2019
@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Nov 8, 2019
Fixes: #30245
PR-URL: #30256
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 8, 2019

Landed in 1eee0b8

@Trott Trott closed this Nov 8, 2019
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
Fixes: #30245
PR-URL: #30256
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
targos pushed a commit that referenced this pull request Dec 1, 2019
Fixes: #30245
PR-URL: #30256
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
Fixes: #30245
PR-URL: #30256
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants