Fix for #1214, linked scoped dependencies should not be overwritten #1970
Conversation
|
Btw potentially yarn allows to have folder starting with so |
|
Also probably while the removal of scoped package it should remove |
Interesting. What do you think would be the best solution then? Testing for the existence of
Yeah, I wasn't sure where the most appropriate place to put this check was. |
|
@tgriesser So it passes removal test without fixes. Simlinks should be carefully handled. Currenlty yarn doesn't remove it if you say remove package it it is simlinked (not sure what should be correct behaviour - should it remove the simlink?) Also there is RFC about |
|
|
||
| for (const maybeLinked of maybeLinkedModules) { | ||
| const linkedPath = path.join(this.linkFolder, maybeLinked); | ||
| const isScopedName = maybeLinked.indexOf('@') === 0; |
There was a problem hiding this comment.
maybeLinked[0] === '@' is probably nicer to read
|
|
||
| const maybeLinkedModules = await fs.readdir(this.linkFolder); | ||
|
|
||
| for (const maybeLinked of maybeLinkedModules) { |
There was a problem hiding this comment.
maybeLinked isn't the best variable name here, how about dir?
| for (const maybeLinked of maybeLinkedModules) { | ||
| const linkedPath = path.join(this.linkFolder, maybeLinked); | ||
| const isScopedName = maybeLinked.indexOf('@') === 0; | ||
| const isScopedContainer = isScopedName && !await fs.exists(path.join(linkedPath, 'package.json')); |
There was a problem hiding this comment.
Hardcoding package.json isn't the best idea since internally we're really agnostic. I'd recommend leaving out this check since you can't have a package named @foo without having a slash in it anyway.
There was a problem hiding this comment.
Though as I know @scoped packages is npm feature.
you can't have a package named @foo without having a slash in it anyway.
Right you can't have this name in package.json to be published to NPM registry, but yarn allows have a folder name that is differs from package.json name, for example:
"@some-package": "file:../some-package"
will create node_modules/@some-package and will treat as usual module.
(npm doesn't allow this) - so maybe this is not a feature, but a "bug"?
|
Updated based on @kittens comments. Not sure what I need to fix the failing windows test though. |
|
@tgriesser I have figured out the problem. In Windows Update Problem is in Windows,
|
|
I tried moving the |
|
@tgriesser, could you rebase first? |
|
@bestander his initial PR was only failing in I tried this too: https://github.com/jprichardson/node-fs-extra#windows |
Scoped dependencies are nested one folder deeper than traditional dependencies, the possiblyExtraneous needed updating to work with the actual dependencies, not the entire scoped folder.
|
Alright, I believe I finally got this one. Needed to ensure the directories were created before the symlink in order for the tests to pass. Also based on the changes made in #2036 - in order to properly remove scoped directories when all modules have been removed, we need to maintain a list of these directories separately and check after the |
|
I seem that it wan't published yet? BTW how is it possible to know if a PR was released in published version? |
|
@whitecolor, no, it is still on master branch, a release is coming next Monday the latest. And then see in what branches it is present.
Currently it is only in master branch, later you'll see |
|
@bestander many thanks for explanation |
Summary
#1214 identifies a problem with scoped linked dependencies being modified after various
yarncommands. After digging into it a bit, it appears the problem is with thepackage-linker.js. ThepossibleExtraneousmodules do not account for the fact that scoped deps are nested underneath their scope name, so the linked scoped deps are not properly added to the list of ignored packages. This leads yarn to replace the currently linked module with the cached version specified in the lockfile.In changing the behavior to properly add these modules, I noticed a test was breaking in
__tests__/commands/remove.js, however I believe this test to be incorrect. It was testing the entire scope folder was removed rather than just the individual package.Test plan
Added a test with the directory structure after a package is
yarn link'ed, with a symlink existing in both.yarn-cache. The test checks that the linked / scopedpackage.jsonis kept intact and that theindex.jsis not copied from the cached module into the linked folder.