Skip to content

fix: resolve workspace paths from cwd when possible#4265

Merged
lukekarrys merged 1 commit intorelease-nextfrom
nlf/auto-workspace-root
Jan 20, 2022
Merged

fix: resolve workspace paths from cwd when possible#4265
lukekarrys merged 1 commit intorelease-nextfrom
nlf/auto-workspace-root

Conversation

@nlf
Copy link
Contributor

@nlf nlf commented Jan 20, 2022 •
edited
Loading

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 •
edited
Loading

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 force-pushed the nlf/auto-workspace-root branch from 107ccf3 to 5e571b7 Compare January 20, 2022 18:41
@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
Merged
@npm-robot npm-robot mentioned this pull request Jan 20, 2022
Closed
@ruyadorno
Copy link
Contributor

ruyadorno commented Jan 25, 2022

relates to: npm/rfcs#343

@ruyadorno ruyadorno mentioned this pull request Jan 25, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lukekarrys lukekarrys lukekarrys approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nlf @ljharb @npm-robot @ruyadorno @lukekarrys