Skip to content

Fix for issue #2840#4989

Merged
kaylie-alexa merged 8 commits into
yarnpkg:masterfrom
Antypurus:master
Dec 4, 2017
Merged

Fix for issue #2840#4989
kaylie-alexa merged 8 commits into
yarnpkg:masterfrom
Antypurus:master

Conversation

@davidreis97

Copy link
Copy Markdown
Contributor

Summary

This is a fix for issue #2840 .

It fixes the issue by comparing the name of the git repo with the entries on the lockfile, instead of comparing the whole git url (which wouldn't match with the version already on the lockfile, since the url would be different).

Test plan

We tested with multiple git repositories, including the ones that were given as an example on the issue page.

@buildsize

buildsize Bot commented Nov 24, 2017

Copy link
Copy Markdown

This change will increase the build size from 10.26 MB to 10.26 MB, an increase of 796 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 886.52 KB 886.21 KB -317 bytes (0%)
yarn-[version].js 3.86 MB 3.86 MB 550 bytes (0%)
yarn-legacy-[version].js 4.01 MB 4.01 MB 555 bytes (0%)
yarn-v[version].tar.gz 891.26 KB 891.05 KB -208 bytes (0%)
yarn_[version]all.deb 667.97 KB 668.18 KB 216 bytes (0%)

@buildsize

buildsize Bot commented Nov 24, 2017

Copy link
Copy Markdown

This change will decrease the build size from 10.26 MB to 10.25 MB, a decrease of 4.04 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 886.52 KB 885.82 KB -724 bytes (0%)
yarn-[version].js 3.86 MB 3.86 MB -1.27 KB (0%)
yarn-legacy-[version].js 4.01 MB 4.01 MB -1.26 KB (0%)
yarn-v[version].tar.gz 891.26 KB 890.65 KB -627 bytes (0%)
yarn_[version]all.deb 667.97 KB 667.77 KB -202 bytes (0%)

@kaylie-alexa

Copy link
Copy Markdown
Member

@davidreis97 Thanks for the PR! can you add a test case for this?

@davidreis97

Copy link
Copy Markdown
Contributor Author

Hi @kaylieEB , I just added a test case in the last commit.

Comment thread src/cli/commands/install.js Outdated
const name = util.removeSuffix(pathname.split('/')[2], '.git');
excludeNames.push(name);
}
} else if (getExoticResolver(pattern)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you instead try using guessName util, so something like

if (getExoticResolver(pattern)) {
   excludeNames.push(guessName(pattern))
}

This way we avoid git-specific logic in the install command file and other exotic resolvers and hosted git resolvers can't take advantage of this fix too :)

Comment thread __tests__/commands/add.js Outdated
// When installing a package with the same name as an existing one but from a different repo,
// the old one should be replaced with the new one in the lock file.
const firstSource = 'https://github.com/davidreis97/example-yarn-package.git';
const secondSource = 'https://github.com/yarnpkg/example-yarn-package.git';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To avoid making requests in the ci, can you add the tarball cache to __tests__/fixtures/request-cache/GET/github.com

@kaylie-alexa kaylie-alexa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎉

@kaylie-alexa kaylie-alexa merged commit 2065988 into yarnpkg:master Dec 4, 2017
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.

2 participants