fix(get-package-name): resolve paths properly before matching #45
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
10xLaCroixDrinker
approved these changes
Jan 2, 2024
giulianok
reviewed
Jan 3, 2024
| .find(([, version]) => version.match(templateName))[0]; | ||
| .find(([, version]) => { | ||
| if (version.startsWith('file:')) { | ||
| const dependencyPath = path.resolve(__dirname, '../../', version.slice(5)); |
There was a problem hiding this comment.
curious, since it's hard to visualize, why ../../ and not ../ or ../../../
There was a problem hiding this comment.
If I remember correctly, __dirname is the current javascript file's dir, so ../../ is reversing out src/utils to get back to the directory where the package.json is. Then the path stored in the package.json with the "file:" prefix is a relative path from that directory.
JAdshead
approved these changes
Jan 5, 2024
smackfu
force-pushed
the
fix/local-install-path-matching
branch
from
9958046 to
bc3e49d
Compare
oneamexbot
added a commit
that referenced
this pull request
Jan 5, 2024
## [1.11.1](v1.11.0...v1.11.1) (2024-01-05) ### Bug Fixes * **get-package-name:** resolve paths properly before matching ([#45](#45)) ([28aefa6](28aefa6))
|
🎉 This PR is included in version 1.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
The current logic for local template packages is:
This seems to only work properly when the local template package filename is in the form
./some/path/package.tgz. If the filename is an absolute path, it doesn't match and errors out. If the filename is a relative path but uses..to go up the tree, it doesn't match and errors. I believe it only works in the one case because the "." in the filename is parsed as a regex and matches any character.This PR changes the logic to fully resolve both the filename in the package.json and the filename passed on the command line, then directly compare them. This should work reliably for all path values.
It also removes the logic that only resolves the path to the tar.gz file when it's a relative path. This isn't needed since the check if the file exists is more reliable and general. This fixes the case where you do not provide a path at all, for instance:
npm init using-template some-template-template-1.0.0.tgzFixes #44