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

Resolves folder options (i.e. --modules-folder) relative to cwd (#7074) #7607

Merged
merged 1 commit into from Oct 7, 2019

Conversation

mbpreble
Copy link
Contributor

@mbpreble mbpreble commented Oct 7, 2019

  • Prevents potentially surprising behavior and data loss

  • Includes integration test verifying that user-reported data loss scenario does not occur

Summary
Motivated by high severity issue #7074, following recommended approach by @arcanis of resolving all folder options relative to the cwd (instead of potentially relative to the nearest project root). Adds an integration test illustrating there are no yarn failures, data loss, or other surprising behaviors in the scenario reported in the issue.

Test plan

It's relatively straightforward to reproduce the reported issue with a current version of yarn:

#! /bin/sh

# Create a project with a lib folder
mkdir -p my-project/my-lib

# My project is some sort of project
echo "{}" > my-project/package.json
echo "I definitely care about this file!" > my-project/IMPORTANT.TXT
cat my-project/IMPORTANT.txt

cd my-project/my-lib

# Specify (using a relative path) that I want to use my-lib as a modules folder, running inside of that folder
yarn add left-pad --modules-folder .

# Hey does my file exist?
cd ..
cat IMPORTANT.txt

Running this shows the yarn command failing, but IMPORTANT.txt having been deleted from the project folder. This does not occur if yarn includes this patch.

…pkg#7074)

* 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 7, 2019

I'm a bit wary of this change since it's technically breaking, but given that the current behavior is quite unexpected and proved dangerous in the wild I'm gonna go ahead and merge it as a bugfix. Hopefully I won't regret it later 😅

@arcanis arcanis merged commit 8a8ce2d into yarnpkg:master Oct 7, 2019
@arcanis
Copy link
Member

arcanis commented Oct 7, 2019

Thanks!

arcanis pushed a commit that referenced this pull request Oct 8, 2019
… (#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 pull request 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 pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants