yarnpkg / yarn

The 1.x line is frozen - features and bugfixes now happen on https://github.com/yarnpkg/berry
https://classic.yarnpkg.com
Other
41.44k stars 2.73k forks source link

"yarn add --module-folder . " deleted ALL of my code (including .git directory) #7074

Closed benvan closed 4 years ago

benvan commented 5 years ago

Do you want to request a feature or report a bug? A bug. A very serious bug. Like, "I'm in serious trouble" serious.

What is the current behavior? You make a directory in your project folder calledlib You run cd lib; yarn add some-module --modules-folder . because you want to install a specific dependency outside of node_modules You get an error telling you a file is missing ... Because yarn deleted all of the files in your project folder (apart from lib and yarn-error.log) You slowly back away from the computer. This is not your day.

If the current behavior is a bug, please provide the steps to reproduce. See above. Although I wouldn't recommend it.

What is the expected behavior? That yarn doesn't delete all of the files in my project.

Please mention your node.js, yarn and operating system version. Yarn: 1.13.0 Node: v10.15.0 OS: macOS 10.14.3

arcanis commented 5 years ago

I'm sorry for your troubles - unfortunately it's kinda expected. The modules-folder settings tell Yarn where are located the packages it should manage - for example by removing the packages that aren't used anymore. If you tell it that your module folder is the current directory, well, it kinda makes sense that it sees your source folders as unused packages.

In this case the only thing we could do would be to tell you that you probably made a mistake (a bit like rm --no-preserve-root), but given that --modules-folder is a very fringe option I'm not sure it's worth the use case.

Do you have a suggestion for a better documentation somewhere that would make this behavior less unexpected?

(As a side note I personally find a bit demoralizing to see you post on HN before even trying to understand what happened. We all need karma in our life but come on ...)

arcanis commented 5 years ago

Giving a second look at your post it appears to me that you were running the command within a subdirectory lib, in which case the behavior might indeed be even more surprising.

When running yarn within a subdirectory of your project, yarn will automatically cd into the closest project it can find and run the command you ran there. It unfortunately doesn't seem to take into account that some command-line arguments should probably be resolved relative to your current cwd rather than the fixed one.

The fix would be to tweak our CLI to resolve some command line arguments relative to the current pwd rather than the fixed one. That would be here:

https://github.com/yarnpkg/yarn/blob/master/src/cli/index.js#L501-L532

benvan commented 5 years ago

Thanks for the swift response @arcanis

The reason for using --modules-folder in this context was for use with aws lambda layers. One of my dependencies is huge, and I wanted to install just that dependency into a separate folder.

It might be worth mentioning - I knew that npm --prefix would do what I wanted - but I'm using yarn. I checked the output of yarn --help and it looked like --modules-folder did the right thing, so I tried it out. I obviously didn't expect it to do what it did.

It's also extremely lucky I didn't run the command again ... because I have a package.json sitting around in my home folder. Which, on second invocation would have been the new "project root" after the first package.json gets deleted...

pedrule commented 5 years ago

I'm same problem in a different context. I want to install in same node_modules folder some dependencies which are flatten ( web-components ). I did that with modules-folder in a sub folder. and now with last version of yarn, same script replace all previously installed modules. It's a dramatical breaking change with last versions if it's not a bug.

benvan commented 5 years ago

At the risk of repeating myself... there is a disaster waiting to happen here.

  1. You have a random ~/package.json sitting around in your home directory
  2. You try to "download" a package by running yarn add pkg --modules-folder . from any directory (e.g. ~/some/unrelated/folder)

If you do this, yarn will delete your entire $HOME directory

I don't think this a hugely uncommon use case. I know I have certainly used npm numerous times in the past to install a particular package into a ~/tmp or ~/scratch directory, for example to inspect a package's config / dist code.

@arcanis I don't mean to sound dramatic ... but this does sound like a serious issue. I respect that this flag may not be oft-used, so the risk of it happening is "low", but the impact here is potentially enormous. Especially if it's true that this behaviour is a recent change (as mentioned by @pedrule )

EDIT: Proof:

image

ls is not found, because /bin no longer exists.

arcanis commented 5 years ago

It's a dramatical breaking change with last versions if it's not a bug.

It got implemented in #4246, in august 2017, and shipped starting with the 1.0. No regression 🙂

I don't mean to sound dramatic ... but this does sound like a serious issue.

I agree - I'll eventually get to fix it, but I'm working on other tasks and if someone is willing to put up a PR to fix this one it would help. A good enough way is to simply wrap all the folder options to do something like path.resolve(cwd, commander.modulesFolder) (+ adding an integration test in __tests__).

benvan commented 5 years ago

@arcanis Yes, the ~--modules-folder~ active directory selection got implemented in august 2017 - but that's not what we're referring to. The change that's relevant here is that of deleting all other files whilst running the command (I'm assuming this is related to "autoclean"?) - which was introduced between v1.9.4 and v1.10.0:

v1.9.4 is fine: image

v1.10.0 breaks: image

benvan commented 5 years ago

I'm pretty sure this is where it broke:

https://github.com/yarnpkg/yarn/pull/6275/files

It looks like "modulesFolder" gets added to registryFolders, which then has its extraneous files/folders deleted.

rally25rs commented 5 years ago

I'm not even sure of a nice way to prevent this from happening. It has always been part of yarn's behavior that we delete extraneous files in node_modules. If you tell it the package directory is ~ then it will always delete non-dependency-package files from there.

The behavior of . becoming relative to the directory that contains package.json instead of the actual pwd /lib is quite unexpected though 😰


Going to tag its as a high priority bug to at least get relative path fixed.

BernieSumption commented 5 years ago

Perhaps a behaviour that could mostly fix this issue is that using --modules-folder either prompt first, or refuse to delete the current working directory or any files in it? This would preserve the common use case of yarn add package-name --modules-folder some-directory but prevent disasters. Would be a breaking change though.

daltonmenezes commented 5 years ago

Same here, I lost all my files right now. It's really sad, because the doc says

Specifies an alternate location for the node_modules directory, instead of the default ./node_modules.

There's nothing warning about that will delete all files in the new location. :(

mbpreble commented 5 years ago

I’d like to work on this. I’ve gotten as far as no longer being able to reproduce this specific issue in my branch.

mbpreble commented 5 years ago

Opened a PR for this, it changes the resolution of several 'folder' options, including --modules-folder. It's no longer possible to reproduce the issue as reported with this change.

arcanis commented 5 years ago

Closing - this fix has been released as part of the 1.19.1. Thanks @mbpreble 🙂