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.

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

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

Closed
benvan opened this issue Feb 28, 2019 · 14 comments
Closed

Comments

@benvan
Copy link

benvan commented Feb 28, 2019

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
Copy link
Member

arcanis commented Feb 28, 2019

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
Copy link
Member

arcanis commented Feb 28, 2019

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
Copy link
Author

benvan commented Feb 28, 2019

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
Copy link

pedrule commented Mar 5, 2019

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
Copy link
Author

benvan commented Mar 6, 2019

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
Copy link
Member

arcanis commented Mar 6, 2019

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
Copy link
Author

benvan commented Mar 6, 2019

@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
Copy link
Author

benvan commented Mar 6, 2019

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
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link
Contributor

mbpreble commented Oct 6, 2019

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 pushed a commit to mbpreble/yarn that referenced this issue Oct 7, 2019
…pkg#7074)

* Prevents potentially surprising behavior and data loss

* Includes integration test verifying that user-reported data loss scenario does not occur
@mbpreble
Copy link
Contributor

mbpreble commented Oct 7, 2019

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 pushed a commit that referenced this issue Oct 7, 2019
… (#7607)

* Prevents potentially surprising behavior and data loss

* Includes integration test verifying that user-reported data loss scenario does not occur
arcanis pushed a commit that referenced this issue Oct 8, 2019
… (#7607)

* Prevents potentially surprising behavior and data loss

* Includes integration test verifying that user-reported data loss scenario does not occur
@arcanis
Copy link
Member

arcanis commented Oct 8, 2019

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

VincentBailly pushed a commit to VincentBailly/yarn that referenced this issue Jun 10, 2020
…pkg#7074) (yarnpkg#7607)

* Prevents potentially surprising behavior and data loss

* Includes integration test verifying that user-reported data loss scenario does not occur
VincentBailly pushed a commit to VincentBailly/yarn that referenced this issue Jun 10, 2020
…pkg#7074) (yarnpkg#7607)

* Prevents potentially surprising behavior and data loss

* Includes integration test verifying that user-reported data loss scenario does not occur
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants