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

add normalize and prepare #32

Merged
merged 5 commits into from
May 15, 2023
Merged

add normalize and prepare #32

merged 5 commits into from
May 15, 2023

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented May 12, 2023

Adds a normalize and prepare function, to replace read-package-json and read-package-json-fast

All the steps can be omitted by an optional parameter. All of the functionality from both libs was ported except for parsing package comments from specific files (the index.js default is still there).

The steps are intentionally undocumented because we may have to tweak them as we find the line between what npm pkg fix should do and what the defaults are.

This brings in parity with `read-package-json-fast`, which is a
normalization method intended for parsing package.json files inside of
an existing node_modules.

Tests were copied straight from that package to ensure functionality was
compatible.  The only tests that weren't copied were the ones testing
down into the bin normalization. That is delegated to a subdependency so
these tests only ensure that *some* normalization is happening.
Eventually as we consolidate our package.json reading libs, bin
normalization can live in this package and be tested here.

Finally, the errors that this package was throwing now include the
metadata from the original errors (such as code) instead of making new
errors with no context.
@wraithgar wraithgar force-pushed the gar/prepare branch 3 times, most recently from e2d289d to 3cb844b Compare May 12, 2023 15:50
@wraithgar wraithgar mentioned this pull request May 12, 2023
@wraithgar wraithgar changed the title feat: add prepare function add normalize and prepare May 12, 2023
This implements most of the logic found in `read-package-json` with the
exception of parsing package comments from specific files.

It also shares all the normalization logic into one method, that
`normalize` and `prepare` then pick and choose which ones to use.

This will allow us to maintain the functionality that
`read-package-json` has of skipping certain steps, but doing so in a
more config-based way instead of passing in functions to run.

All the existing `read-package-json` tests were ported over, and new
ones added to get to 100% coverage.

For now the 'steps' will be undocumented.
@fritzy
Copy link

fritzy commented May 15, 2023

File permission on delete in Windows tests, reran the test and it is fine now. Probably a race condition in the test.

@wraithgar
Copy link
Member Author

flaky windows tests is fixed by https://github.com/npm/cacache/pull/203

Copy link

@fritzy fritzy left a comment

Choose a reason for hiding this comment

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

This is exciting! I wonder if the bin normalizers should be rolled into this.

This is good, and ready to ship, but I do have some questions.
Would steps be more performant as a Set?
Or should steps map to a scoped object of functions, as an array of steps implies order, and iterating through the steps would make sense.
There's a little bit of a smell repeatedly doing searches on array.

@wraithgar
Copy link
Member Author

bin normalizers could only be rolled in if we exported them separately, lots of things use that one separately.

Set only makes sense if we're doing thousands upon thousands of operations. This is half a dozen when we parse a package.json. This is not the time to optimize.

@wraithgar
Copy link
Member Author

One of the lessons learned in the recent ssri/semver optimizations is assumptions are usually wrong, and the only optimizations that should be added are those that got a benchmark and showed a measurable improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants