Fix: Deprecate implicit file: protocol and only allow package dirs
#4257
Conversation
**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.
| import path from 'path'; | ||
|
|
||
| import invariant from 'invariant'; | ||
| import semver from 'semver'; |
There was a problem hiding this comment.
Be careful with this, I think some of our third-party modules (or their dependencies) are sometimes mocked in our tests, so using import will break them. Case in mind: request.
| static prefixMatcher = /^.{,2}\//; | ||
|
|
||
| static isVersion(pattern: string): boolean { | ||
| return super.isVersion.call(this, pattern) || this.prefixMatcher.test(pattern); |
There was a problem hiding this comment.
Maybe update the regex to also match C:\... ?
There was a problem hiding this comment.
Hmm, good catch. I'll go with path.isAbsolute which takes care of it for me.
| @@ -23,6 +24,11 @@ export default class FileResolver extends ExoticResolver { | |||
| loc: string; | |||
|
|
|||
| static protocol = 'file'; | |||
| static prefixMatcher = /^.{1,2}\//; | |||
There was a problem hiding this comment.
Oops, looks like the . isn't escaped, so it's matching any two characters, including single-character scopes like @m.
I have a fix here: #4338
**Summary** The exotic `file-resolver` checks for `.` and `..`, but the regex is missing the escape on the `.`, so *any* two characters count as a file. This includes single-character scope names, like `@s/packagename`. This issue was introduced in #4257. **Test plan** I wasn't sure how packages should be named in `__tests__/fixtures/install/resolutions/exotic-version`, so I have not added add a single-character scoped package reference to test. I don't know of any single-character scopes in the public registry to use as reference, the way `left-pad-1.1.1.tgz` is mirrored in there. Could I just copy `leftpad-1.1.1.tgz` as `@s/leftpad-1.1.1.tgz` and use that?
|
Unfortunately this breaks possibility to install local packages without package.json #3855 :( |
|
@sheerun - yeah that was kind of intentional since this caused lots of headaches and I think this is safer. Is it too much trouble to require people to have a simple |
|
For non-npm packages (like bower's or assets) there isn't package.json available. Also checking for package.json won't solve #4251 if The real fix would be to require local dependencies to have |
|
So, we are doing both right now if I'm not wrong. Are you suggesting making it an either / or check? |
|
Yes, either correct path format, or package.json :) But also deprecate incorrect path next major |
|
@sheerun you have angered the open source gods and now they want a PR. Are you up to the challenge? |
…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.
**Summary** The exotic `file-resolver` checks for `.` and `..`, but the regex is missing the escape on the `.`, so *any* two characters count as a file. This includes single-character scope names, like `@s/packagename`. This issue was introduced in yarnpkg#4257. **Test plan** I wasn't sure how packages should be named in `__tests__/fixtures/install/resolutions/exotic-version`, so I have not added add a single-character scoped package reference to test. I don't know of any single-character scopes in the public registry to use as reference, the way `left-pad-1.1.1.tgz` is mirrored in there. Could I just copy `leftpad-1.1.1.tgz` as `@s/leftpad-1.1.1.tgz` and use that?
yarnpkg#4456) **Summary** Refs yarnpkg#4257. Adds the missing regression test. **Test plan** The new test should pass.
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.jsonfile too. This is still different fromnpm's behavior.Apparently,
npmfetches the package info upfront to favor dist-tagsover 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 certainpoint, 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.