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

fix(modules-folder): Fix --modules-folder handling in several places. #6850

Merged
merged 6 commits into from
Feb 28, 2019

Conversation

rally25rs
Copy link
Contributor

@rally25rs rally25rs commented Dec 23, 2018

Summary

Fixes a few issues with the --modules-folder parameter.

The extraneous file check would always also clean files from node_modules. This no longer happens, only the --modules-folder location is checked.

The yarn check command used to always only check node_modules. Now it will check the --modules-folder location instead. (change to src/config.js)

When running a command or executing a lifecycle script, yarn would build a PATH that included node_modules/.bin first, then add the --modules-folder's /.bin after, which would have led to the wrong binaries being executed. (change to src/util/execute-lifecycle-script.js)

There were a couple places where config.registries[name] was used, instead of config.registryFolders (which would contain the --modules-folder override) so I fixed those. (change to src/cli/commands/autoclean.js and src/cli/commands/check.js)

fixes #5419
fixes #6847
fixes #2784
fixes #6918

Test plan

test added for checking that extraneous file removed from --modules-folder location instead of node_modules.

Not easy to test in our framework but should be manually tested; make sure --modules-folder works as a relative and absolute paths. I think the biggest risk in this PR is that I changed a couple path.joins to path.resolve to handle the case where modules-folder is an absolute path. I think it's working correctly everywhere, but something to keep an eye out for.

Fixes a few issues with the `--modules-folder` parameter.

The extraneous file check would always
also clean files from `node_modules`. This no longer happens, only the `--modules-folder` location
is checked.

The `yarn check` command used to always only check `node_modules`. Now it will check the
`--modules-folder` location instead.

When running a command or executing a lifecycle script, yarn
would build a `PATH` that included `node_modules/.bin` first, then add the `--modules-folder`'s
`/.bin` after, which would have led to the wrong binaries being executed.

fixes yarnpkg#5419 and fixes yarnpkg#6847
@rally25rs
Copy link
Contributor Author

Seeing failures on Appveyor (I tried running it 3 times), but locally on OSX and Win 10 with Node 8 and 10, I the same tests pass.

Can anyone else see if they can reproduce the Appveyor failures?

@levibuzolic
Copy link

Fixes #2784

@navilan
Copy link

navilan commented Jan 18, 2019

Any chance this could be prioritized? This will help us a ton. 🙇

@arcanis arcanis merged commit aebe9e9 into yarnpkg:master Feb 28, 2019
@rally25rs rally25rs deleted the fix-extraneous-file-dir-5419 branch March 27, 2019 12:42
rally25rs added a commit to rally25rs/yarn that referenced this pull request Mar 27, 2019
…nds. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126
arcanis pushed a commit that referenced this pull request Oct 30, 2019
* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in #6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes #7126

* modify chagelog
arcanis pushed a commit that referenced this pull request Nov 22, 2019
* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in #6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes #7126

* modify chagelog
VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
…kg#7151)

* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126

* modify chagelog
VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
…kg#7151)

* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126

* modify chagelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants