-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
install: match git semver ranges #115
Conversation
Let equivalent git semver ranges match. https://npm.community/t/3784
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I think I have some hesitation about using the version
field for git stuff, when resolution happens off tag labels but... I think it's safe to say we consistently assume that package.json's version
field is going to match git tags when using git dependencies, so I think this'll be fine, and I don't see a better way to do this. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite safe:
If the existing child on disk is from the registry, this will result in changing that to be a git-based range not result in updating the child (when it should, as different sources are not the same).
I would suggest that we add an additional restriction to this check:
The child requested should be from the same git repo as the source requested. So the overall logic would be:
"Is this child installed from the same git repo and is its version in our range?"
This also definitely needs a test (not testing this area of code has burned us many times in the past), but I believe can be tested without having to setup a local git server, by injecting a mock of (As an aside, we do actually do some full git functional tests, where we create a local git repo and fire up a server and talk to that. But as I say, I'm hopeful that can be avoided here.) |
Thanks for the feedback! While I'm not entirely familiar with all of the possibilities in that function, this is definitely something I should have caught. I don't really know if I can find much time to work on this and the tests this week, but I'll see (and otherwise, there's next week). |
I added code that should fix the issue you mentioned, and tests. There are some problems with the tests:
|
6ca2f43
to
6d0cc95
Compare
This behaviour was changed somewhere between 6.6.0-next.0 and 6.6.0.
db63b89
to
b09bc8c
Compare
This is correct.
You can! You need to add
I'm confused about this bit. Can you be more specific? |
IIRC, not on stdout.
Yeah I had a hard time understanding it myself now. Basically the test where the installed version is the same version as the requested one, but from a different repository failed, but it also failed without any of my changes so that's probably due to my mocks. I'll look at it later. |
If you're mocking things in npm and want silence (or to capture output), inject a mock for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for writing up tests. 🎉 🍌
I added a mock for |
Let equivalent git semver ranges match. I'm not really sure how to integration-test this without actually cloning git projects at the moment.
See https://npm.community/t/3784.