Fix: Fix numeric versions resolving to local files #4088

BYK · 2017-08-03T15:52:49Z

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.

**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 starts with `./`, `../`. `/`, `~/` (and their `\` versions for Windows) and the matched local entity is a directory. The former part is in line with [NPM specs](https://docs.npmjs.com/files/package.json#local-paths). **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.

bestander · 2017-08-03T16:05:14Z

Afaik "foo": "bar" is legit in npm, and this looks like a breaking Chang anyway

arcanis · 2017-08-03T16:06:56Z

If we want a perfect compatibility we also need to support the following:

{
    "dependencies": {
        "a": "a/b/c"
    }
}

Starting from the third level of depth, NPM tries to install it from ./a/b/c. Note that two levels of depth shouldn't match this codepath, because a/b is a github url (github.com:a/b).

arcanis · 2017-08-03T16:08:47Z

@bestander "foo": "bar" is a valid selector, but it's not resolved on the filesystem. I think it's resolved as a tag of the package.

BYK · 2017-08-03T16:09:08Z

Afaik "foo": "bar" is legit in npm, and this looks like a breaking Chang anyway

Damn, you're right. Even if it is not documented, npm accepts this. The problem is, it installs the package as bar not foo which is not what we do AFAIK.

BYK · 2017-08-03T16:10:53Z

Also, npm doesn't priotize folders over remote versions. I just tried it with having a folder named 1 and npm i foo@1 and it installed the remote version.

BYK · 2017-08-03T16:15:28Z

Looks like the rule is "try file if it is not a valid semver range". Updating accordingly.

arcanis · 2017-08-03T16:21:50Z

Cf my comment about a/b being fetched as a github repository

voxsim · 2017-08-04T08:04:42Z

__tests__/commands/install/integration.js

    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();


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

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.

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

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

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.

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.

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

voxsim · 2017-08-04T08:08:28Z

src/package-request.js

@@ -132,21 +132,27 @@ export default class PackageRequest {
    }
  }

-  async normalizeRange(pattern: string): Promise<string> {
+  async normalizeRange(name: string, pattern: string): Promise<string> {


If I remember correctly we can delete every Promise.resolve because async/await world we should not use them (i.e. It automatically change the result in Promise.resolve).

Do you think we can start to add some unit tests for package-request functions?

I am afraid that every function in this file is not try to adress one responsability and we couple the code in function just for code duplication. One another proof of this is that we have many similar names. If we can't name something, maybe there is a reason.

If I remember correctly we can delete every Promise.resolve because async/await world we should not use them (i.e. It automatically change the result in Promise.resolve).

Oh, right. This now makes it a double promise. Great catch!

Do you think we can start to add some unit tests for package-request functions?

I'd love to but I'd also love to tie them to a "business" purpose. Right now most of these seem to be covered via integration tests. That said I want to do a pass on tests to make them more approachable so may be then?

I am afraid that every function in this file is not try to adress one responsability and we couple the code in function just for code duplication. One another proof of this is that we have many similar names. If we can't name something, maybe there is a reason.

That's a good observation. I think we can give this file a closer look. That said I'd rather not try that in this patch since this PR resolves an important issue and refactoring those may create some back and forths.

voxsim · 2017-08-04T08:11:02Z

@BYK I added some comments, If you think that they are out of scope, ignore them :) and thanks for change the flag 'noLockfile' in 'lockfile' :D

BYK · 2017-08-04T10:13:02Z

@BYK I added some comments, If you think that they are out of scope, ignore them :) and thanks for change the flag 'noLockfile' in 'lockfile' :D

Hey, thanks for the comments! I'll go ahead and fix the Promise.resolve() parts. For more, I'll defer them to follow up to get this important fix in as soon as possible.

cpojer · 2017-08-04T12:07:09Z

src/package-request.js

+    if (!semver.validRange(pattern)) {
+      try {
+        if ((await fs.stat(path.join(this.config.cwd, pattern))).isDirectory()) {
+          return `file:${pattern}`;


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

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

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?

cpojer

I'm approving to unblock but I think @arcanis came back to live and can glance over it before merging.

BYK · 2017-08-04T12:13:21Z

I'm approving to unblock but I think @arcanis came back to live and can glance over it before merging.

👍

arcanis · 2017-08-04T12:31:53Z

src/package-request.js

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


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?

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

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 :)

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.

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

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.

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 :)

**Summary** Fixes #4251. Follow up to #4088. Instead of just checking whether the target is a valid directory, we now check if it contains a `package.json` file too. This is still different from `npm`'s behavior. Apparently, `npm` fetches the package info upfront to favor dist-tags over directories but this comes at a distinct performance penalty and makes static, deterministic resolution impossible so we are now deprecating the implicit `file:` protocol in patterns. After a certain point, we'll remove this code and will require everyone to use `file:` or at least one of the following path identifiers: `./`, `../`. `/`. **Test plan** Updated the existing test for warning check and added a new test for invalid directories.

…4257) **Summary** Fixes #4251. Follow up to #4088. Instead of just checking whether the target is a valid directory, we now check if it contains a `package.json` file too. This is still different from `npm`'s behavior. Apparently, `npm` fetches the package info upfront to favor dist-tags over directories but this comes at a distinct performance penalty and makes static, deterministic resolution impossible so we are now deprecating the implicit `file:` protocol in patterns. After a certain point, we'll remove this code and will require everyone to use `file:` or at least one of the following path identifiers: `./`, `../`. `/`. **Test plan** Updated the existing test for warning check and added a new test for invalid directories.

…arnpkg#4257) **Summary** Fixes yarnpkg#4251. Follow up to yarnpkg#4088. Instead of just checking whether the target is a valid directory, we now check if it contains a `package.json` file too. This is still different from `npm`'s behavior. Apparently, `npm` fetches the package info upfront to favor dist-tags over directories but this comes at a distinct performance penalty and makes static, deterministic resolution impossible so we are now deprecating the implicit `file:` protocol in patterns. After a certain point, we'll remove this code and will require everyone to use `file:` or at least one of the following path identifiers: `./`, `../`. `/`. **Test plan** Updated the existing test for warning check and added a new test for invalid directories.

BYK assigned arcanis Aug 3, 2017

BYK requested review from voxsim, arcanis and bestander August 3, 2017 15:52

BYK mentioned this pull request Aug 3, 2017

EISDIR error due to filename with a single number #3966

Closed

Only use file when it is valid semver

8516fd3

Burak Yigit Kaya added 2 commits August 3, 2017 18:17

Fix tests

4009e62

No idea why Flow is confused here, bind?

5d68a52

voxsim reviewed Aug 4, 2017

View reviewed changes

Remove obsolete Promise.resolve() calls

eb93c50

cpojer reviewed Aug 4, 2017

View reviewed changes

cpojer approved these changes Aug 4, 2017

View reviewed changes

Remove obsolete name argument

9b83d3f

arcanis reviewed Aug 4, 2017

View reviewed changes

arcanis approved these changes Aug 4, 2017

View reviewed changes

BYK merged commit 39d6fe2 into master Aug 4, 2017

BYK deleted the one-bug branch August 4, 2017 13:35

BYK mentioned this pull request Aug 25, 2017

Fix: Deprecate implicit file: protocol and only allow package dirs #4257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Fix numeric versions resolving to local files #4088

Fix: Fix numeric versions resolving to local files #4088

BYK commented Aug 3, 2017 •

edited

Loading

bestander commented Aug 3, 2017

arcanis commented Aug 3, 2017

arcanis commented Aug 3, 2017

BYK commented Aug 3, 2017

BYK commented Aug 3, 2017

BYK commented Aug 3, 2017

arcanis commented Aug 3, 2017

voxsim Aug 4, 2017

BYK Aug 4, 2017

cpojer Aug 4, 2017

BYK Aug 4, 2017

cpojer Aug 4, 2017

BYK Aug 4, 2017

cpojer Aug 4, 2017

voxsim Aug 4, 2017

BYK Aug 4, 2017

voxsim commented Aug 4, 2017

BYK commented Aug 4, 2017

cpojer Aug 4, 2017

cpojer Aug 4, 2017

BYK Aug 4, 2017

cpojer left a comment

BYK commented Aug 4, 2017 •

edited

Loading

arcanis Aug 4, 2017

arcanis Aug 4, 2017

BYK Aug 4, 2017

BYK Aug 4, 2017

BYK Aug 4, 2017

arcanis Aug 4, 2017

arcanis Aug 4, 2017

Fix: Fix numeric versions resolving to local files #4088

Fix: Fix numeric versions resolving to local files #4088

Conversation

BYK commented Aug 3, 2017 • edited Loading

bestander commented Aug 3, 2017

arcanis commented Aug 3, 2017

arcanis commented Aug 3, 2017

BYK commented Aug 3, 2017

BYK commented Aug 3, 2017

BYK commented Aug 3, 2017

arcanis commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxsim commented Aug 4, 2017

BYK commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

BYK commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Aug 3, 2017 •

edited

Loading

BYK commented Aug 4, 2017 •

edited

Loading