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

install: match git semver ranges #115

Merged
merged 6 commits into from
Feb 18, 2019
Merged

install: match git semver ranges #115

merged 6 commits into from
Feb 18, 2019

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Dec 13, 2018

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.

Let equivalent git semver ranges match.

https://npm.community/t/3784
@larsgw larsgw requested a review from a team as a code owner December 13, 2018 11:09
Copy link
Contributor

@zkat zkat left a 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!

@zkat zkat added the semver:patch semver patch level for changes label Jan 7, 2019
Copy link
Contributor

@iarna iarna left a 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?"

@iarna
Copy link
Contributor

iarna commented Jan 7, 2019

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 lib/fetch-package-metadata (we use require-inject for this).

(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.)

@larsgw
Copy link
Contributor Author

larsgw commented Jan 7, 2019

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).

@larsgw
Copy link
Contributor Author

larsgw commented Jan 18, 2019

I added code that should fix the issue you mentioned, and tests. There are some problems with the tests:

  • I don't think I can mock part of npm while using common-tap.js
  • Using npm directly works, but I don't think I can completely silence output, like installs "added n new packages"
  • Changing the test to make sure repository sources are the same failed due to outright removing a dependency. This also happens without any of the changes I applied, and seems to be related to some lack of metadata in the manifest, but I couldn't pinpoint the part were it's removed.

@zkat zkat force-pushed the release-next branch 2 times, most recently from 6ca2f43 to 6d0cc95 Compare January 18, 2019 19:17
This behaviour was changed somewhere between 6.6.0-next.0 and 6.6.0.
@zkat zkat force-pushed the release-next branch 3 times, most recently from db63b89 to b09bc8c Compare January 23, 2019 18:36
@zkat
Copy link
Contributor

zkat commented Jan 28, 2019

* I don't think I can mock part of npm while using `common-tap.js`

This is correct.

* Using npm directly works, but I don't think I can completely silence output, like `install`s "added n new packages"

You can! You need to add "--loglevel", "silent" to the arguments. :)

* Changing the test to make sure repository sources are the same failed due to outright removing a dependency. This also happens without any of the changes I applied, and seems to be related to some lack of metadata in the manifest, but I couldn't pinpoint the part were it's removed.

I'm confused about this bit. Can you be more specific?

@larsgw
Copy link
Contributor Author

larsgw commented Jan 28, 2019

You can! You need to add "--loglevel", "silent" to the arguments. :)

IIRC, not on stdout.

I'm confused about this bit. Can you be more specific?

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.

@iarna
Copy link
Contributor

iarna commented Jan 29, 2019

If you're mocking things in npm and want silence (or to capture output), inject a mock for lib/utils/output.js

@zkat zkat removed the in-progress label Feb 11, 2019
Copy link
Contributor

@zkat zkat left a 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. 🎉 🍌

@larsgw
Copy link
Contributor Author

larsgw commented Feb 11, 2019

I added a mock for utils/output.js. It seems to work but I broke the tests on my end (a290a67) so that it works with the current release-next, so I cannot know for sure until CI is done.

@zkat zkat added semver:minor new backwards-compatible feature and removed semver:patch semver patch level for changes labels Feb 18, 2019
@zkat zkat merged commit 8047b19 into npm:release-next Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants