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.

Sign up for GitHub

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

Fix: Fix numeric versions resolving to local files #4088

Merged
merged 6 commits into from
Aug 4, 2017
Merged

Fix: Fix numeric versions resolving to local files #4088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd9ef25
Fix: Fix numeric versions resolving to local files
Aug 3, 2017
8516fd3
Only use file when it is valid semver
Aug 3, 2017
4009e62
Fix tests
Aug 3, 2017
5d68a52
No idea why Flow is confused here, bind?
Aug 3, 2017
eb93c50
Remove obsolete `Promise.resolve()` calls
Aug 4, 2017
9b83d3f
Remove obsolete `name` argument
Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions __tests__/commands/install/__snapshots__/integration.js.snap
View file
Open in desktop
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
View file
Open in desktop
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem here is rejects, Do we use this somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because of that. This was introduced by Jest 20 and @onurtemizkan upgraded flow type definitions and used it in one of his patches.

I think Flow gets confused by how runInstall is defined by bind and cannot determine the output type of expect here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend turning it into an async function and awaiting the expect call. Looks a bit cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpojer you don't like returning promises from tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not explicitly, no. Turning the function into an async function will implicitly return a promise, which Jest then uses to make the test wait for the promise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright. I'll defer this to a later diff tho since it would require a large number of changes in this file (to be consistent) which are not very related to this particular problem. I know I did the lockfile: false changes but it was required to make sure tests were running correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me, file it under the post-post-1.0 tag ;P

});

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 All @@ -492,7 +505,7 @@ test.concurrent('install file: dedupe dependencies 2', (): 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 @@ -574,7 +587,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 @@ -746,7 +759,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 @@ -761,7 +774,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 added __tests__/fixtures/install/install-file-as-default-no-file/bar
View file
Open in desktop
Empty file.
5 changes: 5 additions & 0 deletions __tests__/fixtures/install/install-file-as-default-no-file/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"foo": "bar"
}
}
1 change: 1 addition & 0 deletions __tests__/fixtures/install/install-file-as-default-no-semver/1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foobar;
5 changes: 5 additions & 0 deletions __tests__/fixtures/install/install-file-as-default-no-semver/1/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "1",
"version": "0.0.0",
"main": "index.js"
}
5 changes: 5 additions & 0 deletions __tests__/fixtures/install/install-file-as-default-no-semver/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"foo": "1"
}
}
4 changes: 4 additions & 0 deletions __tests__/fixtures/install/install-file-as-default-no-semver/yarn.lock
View file
Open in desktop
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 added __tests__/fixtures/request-cache/GET/registry.yarnpkg.com/foo.bin
View file
Open in desktop
Binary file not shown.
Binary file added __tests__/fixtures/request-cache/GET/registry.yarnpkg.com/foo/-/foo-1.0.0.tgz.bin
View file
Open in desktop
Binary file not shown.
14 changes: 10 additions & 4 deletions src/package-request.js
View file
Open in desktop
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()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this check is the right one:

  • It seems that NPM reports "Could not install from "a/b/c/d" as it does not contain a package.json file" even with an unexisting path
  • It should work not only if the path is a directory, but also if it is a tarball

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Is there something preventing us from just fallbacking to the filesystem if the pattern doesn't match validRange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know if NPM supports tarballs. I'm fine with adding a bit more checks. My initial approach was a lot stricter actually: read the package.json file and make sure the name matches the wanted package name, hence that extra name argument in the previous version.

That said when we try to read a package manifest, we automatically fallback to the directory name as package name. This, combined with NPM being quite liberal about paths, lead to this final approach which just checks if it is a directory and it is not a valid semver range.

Awaiting further opinions :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there something preventing us from just fallbacking to the filesystem if the pattern doesn't match validRange?

Except for the fact that I find it very dangerous (even this current version), I don't think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, here's one: if there's a typo, it would make the error message quite cryptic.

Copy link
Member

Choose a reason for hiding this comment

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

make sure the name matches the wanted package name, hence that extra name argument in the previous version.

Yeah we shouldn't check that, especially since we have a feature to rename packages, so we might end up installing packages under a different name than their original one.

Except for the fact that I find it very dangerous (even this current version), I don't think so.

How so? It seems less dangerous to me, since it means that whatever the filesystem is, Yarn would behave the same way on two different systems. Otherwise, there's a risk it will fallback to fetching the package from different ways if the file doesn't exists, instead of reporting about the missing file.

Copy link
Member

Choose a reason for hiding this comment

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

That being said, I checked on NPM and it seems that they just silently ignore "foo": "foo.tgz" ... so I guess this PR should be fine :)

return `file:${pattern}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we warn if this happens? I kinda feel like this file pattern isn't great.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I'm saying, we should support it, but hint at the user that it's silly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific? Do you think we should warn people when they omit the file: prefix? I think that's reasonable but would it be a warning or info? And what if "they just prefer it that way?". May be this should be controlled this via a config flag: warn-unsafe-file false?

}
} catch (err) {
// pass
}
}

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

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