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

Support for semver hash in hosted git dependency #5858

Merged
merged 5 commits into from
May 29, 2018

Conversation

lenkan
Copy link
Contributor

@lenkan lenkan commented May 23, 2018

Summary

npm supports referencing hosted github dependencies in the format github:user/repo#semver:^1.6.0. This is currently not supported by yarn as it resolves the hash into #semver/^1.6.0. Note the added / instead of the :.

Thus, yarn outputs the error: "Couldn't find match for "semver/^1.6.0" in ...."

Motivation
Given an empty yarn package:

// package.json 
{
  "name": "tjosan",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "yarn": "github:yarnpkg/yarn#semver:^1.6.0"
  }
}

When running yarn:

yarn

It errors with:

error Couldn't find match for "semver/^1.6.0" in ....

Whereas npm resolved the dependency without errors.

Test plan

Given an empty yarn package:

// package.json 
{
  "name": "tjosan",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "yarn": "github:yarnpkg/yarn#semver:^1.6.0"
  }
}

When running yarn:

yarn

It should resolve the dependencies:

yarn install v1.6.0
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
Done in x.xxs.

@buildsize
Copy link

buildsize bot commented May 23, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 917.28 KB 915.63 KB -1.65 KB (0%)
yarn-[version].js 3.97 MB 3.96 MB -7.71 KB (0%)
yarn-legacy-[version].js 4.13 MB 4.12 MB -9.14 KB (0%)
yarn-v[version].tar.gz 922.34 KB 920.96 KB -1.39 KB (0%)
yarn_[version]all.deb 681.06 KB 680.34 KB -744 bytes (0%)

@lenkan
Copy link
Contributor Author

lenkan commented May 23, 2018

It seems like older node doesn't support the negative lookahead regex, I will look into it if this is something you want.

@BYK
Copy link
Member

BYK commented May 24, 2018

Thanks a lot for the patch! I'll let @rally25rs and @imsnif look into this since they have worked on similar pieces recently.

It seems like older node doesn't support the negative lookahead regex, I will look into it if this is something you want.

Tests seem to be failing even on Node 10 so we need to get those fixed before we can merge or even review this.

@BYK BYK requested review from rally25rs and imsnif May 24, 2018 12:06
BYK
BYK previously requested changes May 24, 2018
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Rejecting for CI failures.

@kruncher
Copy link

I too ran into this issue today when attempting to use yarn in a project.

@lenkan
Copy link
Contributor Author

lenkan commented May 26, 2018

I tried to figure out an easy way to do it without the negative lookbehind regex. But I couldn't understand the existing method too well. From the tests, all I could gather was that we should find the user, repo and hash from a git reference string. So I thought, isn't that just to:

  1. Strip hash
  2. Strip anything leading up to the user name.
  3. Split the remaining parts on / where the last bit is the repo, and the second last is the user?

All the initial tests passed, I added a bunch of new test cases. Did I miss any test cases? I can't see what some of the string splits were achieving.

@imsnif imsnif self-assigned this May 28, 2018
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Hey @lenkan - this LGTM.

I feel it's cleaner, easier to understand while keeping all previous functionality and adding this new use case.
I also think the new tests are a step up!

My only issue is with the tests being generated, as my personal opinion is that they're a little harder to work with when debugging (as opposed to "normal" imperative tests) - but since the code base is filled with such examples, I feel it's fine to leave them in. :)

Thanks for this!

@imsnif imsnif dismissed BYK’s stale review May 29, 2018 09:21

Concerns addressed

@imsnif imsnif merged commit 643deaa into yarnpkg:master May 29, 2018
@kruncher
Copy link

Many thanks! Quick question, how long until this reaches the npm release channel?

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.

4 participants