fix(arborist) _findMissingEdges missing dependency due to inconsistent path separators #4261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dependencies nested in node_modules triggers an issue with a file path used as a cache key in the _findMissingEdges function on Windows due to paths with mixed path separtors.
Normalize path to avoid mixed path separators on Windows causing a cache miss for paths that refer to the same thing. This would otherwise cause a new node with the path of an existing node to be created which in turns deletes the existing node and its children. However, the children will not be loaded again from the file system due to the _actualTreeLoaded check in _loadFSTree.
fritzy
changed the title
|
no statistically significant performance changes detected timing results
|
|
@fritzy Do I need to do something about the test failures due to |
|
Hey @fritzy, is there something specific blocking this PR? Can I trigger a rerun of the test suite that failed due to what looked like a timeout issue? |
wraithgar
approved these changes
Mar 14, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
THIS PR HAS BEEN MIGRATED
Originally at npm/arborist#363
Originally by @salvadorj
When installing a package with bundled dependencies with npm it can end up missing some dependencies that were bundled and attempt to fetch from the remote registry (see #2757). This issue only happens on Windows and it's caused by
_findMissingEdgesmissing one of the bundled dependencies. The reason it only happens on Windows is because of this lookup inthis[_cache]with a path that contains mixed path separators:https://github.com/npm/arborist/blob/02f295f2d0caa6de2d3235c6eb1c180a8afa0f6f/lib/arborist/load-actual.js#L433-L435
On macOS we get a result back and avoid creating a new node. On Windows we get no result since the key in the cache is using backslashes but the lookup key has both backslashes and forward slashes. This results in a new node being created with the path of an existing node which in turn removes the existing node and its children from the tree. The children are however not loaded from the file system again because of this check:
https://github.com/npm/arborist/blob/02f295f2d0caa6de2d3235c6eb1c180a8afa0f6f/lib/arborist/load-actual.js#L353-L357
The end result is an unmet dependency and npm attempting to fetch it from the registry instead of using the bundled dependency.
I've changed the cache key by normalizing the path to avoid replacing existing nodes. I've setup a minimal test case where the difference in behavior (with and without normalization) shows up as an error on the outgoing edge of a node.
References
Fixes #2757