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: resolve workspace paths from cwd when possible #4265

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Jan 20, 2022

these are changes that are required along with landing and publishing npm/config#28 (and updating it here) to enable automatic detection of a workspace root.

this fix stands on its own, however. if a user had the structure

/package.json
/workspaces/a/package.json

and they ran npm test -w ./a from within /workspaces/, they would currently get an error. this corrects that behavior so we resolve the path for a based on process.cwd() instead of the workspace root.

to disable this feature currently a user must run npm --prefix=./ npm --no-workspaces to skip the prefix auto detection. the workspaces flag must be part of the environment or command line flags and not in a file to work.

the above comment is not directly related to this pull request, but i wanted to keep it here for posterity

@ljharb
Copy link
Contributor

ljharb commented Jan 20, 2022

Would this encourage use of prefix in npmrc or the env? using prefix breaks nvm and sometimes termux, so it’d be ideal not to increase usage of it.

@npm-robot
Copy link
Contributor

npm-robot commented Jan 20, 2022

found 20 benchmarks with statistically significant performance improvements

  • app-large: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
  • app-medium: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 57.522 ±1.47 30.050 ±0.14 28.970 ±16.03 19.818 ±1.51 2.787 ±0.01 2.995 ±0.06 2.408 ±0.02 11.233 ±0.25 2.310 ±0.03 3.168 ±0.01
#4265 0.394 ±0.00 0.393 ±0.00 0.392 ±0.00 0.386 ±0.01 0.387 ±0.00 0.392 ±0.01 0.394 ±0.01 0.393 ±0.00 0.391 ±0.00 0.426 ±0.02
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 40.191 ±4.28 22.135 ±0.10 12.959 ±0.85 13.651 ±0.27 2.549 ±0.02 2.519 ±0.03 2.820 ±0.84 8.479 ±0.44 2.232 ±0.02 3.006 ±0.03
#4265 0.444 ±0.02 0.428 ±0.00 0.422 ±0.01 0.390 ±0.00 0.387 ±0.00 0.388 ±0.00 0.396 ±0.00 0.390 ±0.00 0.390 ±0.00 0.389 ±0.00

@nlf
Copy link
Contributor Author

nlf commented Jan 20, 2022

Would this encourage use of prefix in npmrc or the env? using prefix breaks nvm and sometimes termux, so it’d be ideal not to increase usage of it.

very valid point. i've tweaked the changes to @npmcli/config so that --no-workspaces on the cli will also disable this check

@nlf nlf changed the title feat: auto workspace root fix: allow finding workspaces relative from process.cwd() instead of workspace root Jan 20, 2022
@nlf nlf changed the title fix: allow finding workspaces relative from process.cwd() instead of workspace root fix: resolve workspace paths from cwd when possible Jan 20, 2022
@nlf nlf marked this pull request as ready for review January 20, 2022 18:50
@nlf nlf requested a review from a team as a code owner January 20, 2022 18:50
@nlf
Copy link
Contributor Author

nlf commented Jan 20, 2022

moved to ready for review since this can land independently of the @npmcli/config changes (but needs to land before updating @npmcli/config)

@lukekarrys lukekarrys merged commit 14a3d95 into release-next Jan 20, 2022
@lukekarrys lukekarrys deleted the nlf/auto-workspace-root branch January 20, 2022 20:37
@lukekarrys lukekarrys mentioned this pull request Jan 20, 2022
@ruyadorno
Copy link
Contributor

relates to: npm/rfcs#343

@ruyadorno ruyadorno mentioned this pull request Jan 25, 2022
4 tasks
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.

5 participants