Skip to content

Commit

Permalink
Fix: Fix numeric versions resolving to local files (#4088)
Browse files Browse the repository at this point in the history
**Summary**

Fixes #4013. We were blindly priotizing local files (even if they
are not directories) if there's anything on the file system matching
a pattern for a package. This patch ensures that we only do this if
the pattern is not a valid semver range and the matched local entity is a directory.
**Test plan**

Added a new unit test and modified an incorrect old one. Also manually
verified that the issue described in #4013 does not happen anymore.
  • Loading branch information
BYK authored Aug 4, 2017
1 parent 280b6eb commit 39d6fe2
Show file tree
Hide file tree
Showing 11 changed files with 68 additions and 11 deletions.
18 changes: 18 additions & 0 deletions __tests__/commands/install/__snapshots__/integration.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`install-file-as-default-no-semver 1`] = `
"{
\\"author\\": \\"AJ ONeal <coolaj86@gmail.com> (http://coolaj86.info)\\",
\\"name\\": \\"foo\\",
\\"description\\": \\"A test module with no \`main\`, \`lib\`, or \`dependencies\` specified\\",
\\"version\\": \\"1.0.0\\",
\\"repository\\": {
\\"type\\": \\"git\\",
\\"url\\": \\"git://github.com/coolaj86/node-pakman.git\\"
},
\\"engines\\": {
\\"node\\": \\">= v0.2\\"
}
}
"
`;
27 changes: 20 additions & 7 deletions __tests__/commands/install/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ test.concurrent('root install with optional deps', (): Promise<void> => {
});

test.concurrent('install file: protocol with relative paths', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-file-relative', async config => {
return runInstall({}, 'install-file-relative', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'root-a', 'index.js'))).toEqual('foobar;\n');
});
});
Expand Down Expand Up @@ -459,17 +459,30 @@ test.concurrent('install file: link file dependencies', async (): Promise<void>
});

test.concurrent('install file: protocol', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-file', async config => {
return runInstall({lockfile: false}, 'install-file', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('foobar;\n');
});
});

test.concurrent('install with file: protocol as default', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-file-as-default', async config => {
return runInstall({}, 'install-file-as-default', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('foobar;\n');
});
});

test.concurrent("don't install with file: protocol as default if target is a file", (): Promise<void> => {
// $FlowFixMe
return expect(runInstall({lockfile: false}, 'install-file-as-default-no-file')).rejects.toBeDefined();
});

test.concurrent("don't install with file: protocol as default if target is valid semver", (): Promise<void> => {
return runInstall({}, 'install-file-as-default-no-semver', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'package.json'))).toMatchSnapshot(
'install-file-as-default-no-semver',
);
});
});

// When local packages are installed, dependencies with different forms of the same relative path
// should be deduped e.g. 'file:b' and 'file:./b'
test.concurrent('install file: dedupe dependencies 1', (): Promise<void> => {
Expand Down Expand Up @@ -505,7 +518,7 @@ test.concurrent('install file: dedupe dependencies 3', (): Promise<void> => {
});

test.concurrent('install everything when flat is enabled', (): Promise<void> => {
return runInstall({noLockfile: true, flat: true}, 'install-file', async config => {
return runInstall({lockfile: false, flat: true}, 'install-file', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('foobar;\n');
});
});
Expand Down Expand Up @@ -587,7 +600,7 @@ test.concurrent('install should run install scripts in the order of dependencies
});

test.concurrent('install with comments in manifest', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-with-comments', async config => {
return runInstall({lockfile: false}, 'install-with-comments', async config => {
expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('foobar;\n');
});
});
Expand Down Expand Up @@ -759,7 +772,7 @@ test.concurrent('offline mirror can be disabled locally', (): Promise<void> => {

// sync test because we need to get all the requests to confirm their validity
test('install a scoped module from authed private registry', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-from-authed-private-registry', async config => {
return runInstall({}, 'install-from-authed-private-registry', async config => {
const authedRequests = request.__getAuthedRequests();

expect(authedRequests[0].url).toEqual('https://registry.yarnpkg.com/@types%2flodash');
Expand All @@ -774,7 +787,7 @@ test('install a scoped module from authed private registry', (): Promise<void> =
});

test('install a scoped module from authed private registry with a missing trailing slash', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-from-authed-private-registry-no-slash', async config => {
return runInstall({}, 'install-from-authed-private-registry-no-slash', async config => {
const authedRequests = request.__getAuthedRequests();

expect(authedRequests[0].url).toEqual('https://registry.yarnpkg.com/@types%2flodash');
Expand Down
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"foo": "bar"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foobar;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "1",
"version": "0.0.0",
"main": "index.js"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"foo": "1"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1
"foo@file:bar":
version "0.0.0"
Binary file not shown.
Binary file not shown.
14 changes: 10 additions & 4 deletions src/package-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,20 @@ export default class PackageRequest {

async normalizeRange(pattern: string): Promise<string> {
if (pattern.indexOf(':') > -1 || pattern.indexOf('@') > -1 || getExoticResolver(pattern)) {
return Promise.resolve(pattern);
return pattern;
}

if (await fs.exists(path.join(this.config.cwd, pattern))) {
return Promise.resolve(`file:${pattern}`);
if (!semver.validRange(pattern)) {
try {
if ((await fs.stat(path.join(this.config.cwd, pattern))).isDirectory()) {
return `file:${pattern}`;
}
} catch (err) {
// pass
}
}

return Promise.resolve(pattern);
return pattern;
}

async normalize(pattern: string): any {
Expand Down

0 comments on commit 39d6fe2

Please sign in to comment.