yarnpkg / yarn

The 1.x line is frozen - features and bugfixes now happen on https://github.com/yarnpkg/berry
https://classic.yarnpkg.com
Other
41.44k stars 2.73k forks source link

Yarn offline mirror filename calculation broken for NPM Enterprise #7199

Closed jmillikin-stripe closed 5 years ago

jmillikin-stripe commented 5 years ago

What is the current behavior?

Yarn's offline mirror names downloaded tarballs based on their URL. For URLs that contain an organizational scope (like @babel/), the tarball filenames include this scope. This calculation is based on a regex that does not properly match tarball URLs generated by NPM Enterprise.

Relevant code from src/fetchers/tarball-fetcher.js:

const RE_URL_NAME_MATCH = /\/(?:(@[^/]+)\/)?[^/]+\/-\/(?:@[^/]+\/)?([^/]+)$/;

// getTarballMirrorPath()
const {pathname} = url.parse(this.reference);
const match = pathname.match(RE_URL_NAME_MATCH);

const [, scope, tarballBasename] = match;
packageFilename = scope ? `${scope}-${tarballBasename}` : tarballBasename;

If the current behavior is a bug, please provide the steps to reproduce.

Example: for package @cypress/listr-verbose-renderer, the URL pathnames are:

An online regex visualizer such as https://www.debuggex.com/ can be used to verify that Yarn's current RE_URL_NAME_MATCH does not match the NPM-E format.

What is the expected behavior?

Yarn's offline mirror should work for files stored in NPM Enterprise. The lack of parser support causes conflicts when two packages have the same basename.

Please mention your node.js, yarn and operating system version.

This is reproducible in Yarn 1.15.2 (the most recent release). It is independent of OS or Node.JS version.

arcanis commented 5 years ago

Npm decided to stray away from their own convention for no real gain. They did this without consulting anyone, and actually stayed silent when I reported to them that hey, it's not a great idea because it actually prevents everyone from writing better tools (cf #5892). As such, I decided that I wouldn't even try to support npm enterprise myself.

arcanis commented 5 years ago

(I'm actually a bit more abrasive that I'd like - I'm willing to merge a PR to fix this in the v1. However, in the v2 it will definitely require npm enterprise users to manually toggle a setting, since that would otherwise downgrade the experience for everyone).

jmillikin-stripe commented 5 years ago

The fix for v1 is trivial, see above PR. I agree it's silly that registry.npmjs.org and NPM Enterprise have different conventions here, but fixing that is much more challenging than letting Yarn work around it.

FWIW, in Yarn v2 I strongly recommend changing the offline cache dir calculation to use the package information rather than the tarball URL. That would avoid the issue of URL formats entirely.

arcanis commented 5 years ago

The v2 has a slightly different way - we hash a remote reference (ie lodash@npm:1.2.3) rather than the tarball url. It's perfect because we just have to construct the tarball url at install time based on the convention + the registry url + the remote reference ... except if the convention changes, in which case we would have to make a query to the remote registry to figure out the true tarball url ... 🙄

I'm going to take a look at your PR tomorrow, and it's likely to get released by the end of the week - please ping me if I forget.

jmillikin-stripe commented 5 years ago

I'm going to take a look at your PR tomorrow, and it's likely to get released by the end of the week - please ping me if I forget.

Gentle ping -- how's the release coming along?

arcanis commented 5 years ago

@jmillikin-stripe I'm currently releasing the 1.16 in the RC channels - sorry for the delay! 🙂

arcanis commented 5 years ago

Available - you can install it via npm install -g yarn@rc or any other other install strategy listed on our install page.