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

feat(workspaces): --include-workspace-root #3587

Closed
wants to merge 107 commits into from

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Jul 27, 2021

Adds a new config item that includes the workspace root when running
non-arborist commands (i.e. repo, version, publish). Arborist will need
to be udpated to look for this flag to change its behavior to include
the workspace root for its functions.

This also changes --workspaces to a trinary, so that setting it to false
will explicitly exclude workspaces altogether. This is also going to
require an arborist change so that it ignores workspaces altogether.

Closes npm/statusboard#371

@wraithgar wraithgar requested a review from a team as a code owner July 27, 2021 22:52
@wraithgar
Copy link
Member Author

Probably want to add to usage and/or docs for this.

@wraithgar
Copy link
Member Author

This still doesn't solve the problem of "run this command under the context of all workspaces and also my local workspace"

Also, given the fact that right now including a workspace also includes all ITS workspaces, we may need to either rethink this, or make sure we're very clear in the docs what this is for.

@ljharb
Copy link
Collaborator

ljharb commented Jul 28, 2021

Using . certainly shouldn’t also include transitive workspaces, that defeats the entire point of using it.

@wraithgar
Copy link
Member Author

Using . certainly shouldn’t also include transitive workspaces, that defeats the entire point of using it.

It doesn't, but there is a bit of a mental model for arborist-related commands that if I include a workspace (for instance during install) then its workspaces are also included). I don't think this is a problem.

Current thinking is:

  • -ws adds all workspaces to the current context i.e. npm run test -ws runs tests in all workspaces.
  • -ws by default does not include the current project as a context (this is the current behavior already)
  • -w . can be combined with -ws to effect the desired run this command in all my workspaces AND my root project
  • -w . can be used in isolation (this is most needed for arborist related commands) to exclude all workspaces from the task

It does seem that there is already a bit of a confusion between the mental models for arborist-related things, and the other commands e.g. ls, run, repo. It is what it is at this point we just have to try to not make it worse if we can help it.

@ljharb
Copy link
Collaborator

ljharb commented Jul 28, 2021

I like the current thinking.

Personally i prefer the long form for all my CLI args; it makes things much clearer to think about.

@wraithgar
Copy link
Member Author

Treating . as a workspace has lots of other unintended problems because we don't actually ever treat it as a workspace.

We're gonna instead use a specific --include-workspace-root flag (still needs a good shorthand).

@ljharb
Copy link
Collaborator

ljharb commented Jul 28, 2021

The . is particularly nice for configs - i use it in lerna - but for cli invocations this should work the same.

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Aug 5, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 35 milestone Aug 9, 2021
Adds a new config item that includes the workspace root when running
non-arborist commands (i.e. repo, version, publish).  Arborist will need
to be udpated to look for this flag to change its behavior to include
the workspace root for its functions.

This also changes --workspaces to a trinary, so that setting it to false
will explicitly exclude workspaces altogether.  This is also going to
require an arborist change so that it ignores workspaces altogether.
@wraithgar wraithgar changed the title fix(workspaces): allow for -w . feat(workspaces): --include-workspace-root Aug 11, 2021
@wraithgar
Copy link
Member Author

Default:
  Command is ran under the context of the root project only
  Workspaces are used in node_modules if the name matches the required dependency

--workspaces
  Command is ran under the context of every workspace
  Command is not ran under the context fo the root project
  Workspaces are used in node_modules if the name matches the required dependency

--workspace x
  Command is ran under the context of workspace x
  Command is not ran under the context fo the root project
  Workspaces are used in node_modules if the name matches the required dependency


--workspace x --include-workspace-root
  Command is ran under the context of workspace x
  Command is also ran under the context of the root project
  Workspaces are used in node_modules if the name matches the required dependency


--no-workspaces
  Command is ran under the context of the root project only
  Workspaces are not used in node_modules

--no-workspaces --include-workspace-root
  Redundant

--no-workspaces --workspace x
  Throws an exception

@wraithgar
Copy link
Member Author

Exception throw isn't done yet, just realized.

wraithgar and others added 11 commits August 11, 2021 10:53
  * [#3632] Fix "cannot read property path of null" error in 'npm
    dedupe'
  * fix(shrinkwrap): always set name on the root node
Name is always now set on the root node

See: npm/arborist#310
`leven` dropped support for node10 and we still currently have to support
it.  Moved to https://github.com/ka-weihe/fastest-levenshtein

Originally discussed in #2403, but the
did-you-mean lib moved quite a bit since then and there were conflicts
so I made a new PR

PR-URL: #3640
Credit: @wraithgar
Close: #3640
Reviewed-by: @nlf
The content in this portion of the docs is auto-generated.

PR-URL: #3654
Credit: @wraithgar
Close: #3654
Reviewed-by: @nlf
This should prevent the kind of thing we've seen where people edit the
generated docs, as in #3654 and #3630, and provide them with a helpful
pointer so they put the config documentation changes in the right place.
When we accidentally edit the auto-generated portions of the docs, this
will catch the error and cause CI to fail.

Later phase automated safety check that the early-stage human commenting
in the last commit also addresses.

Re: #3654
Re: #3630

PR-URL: #3655
Credit: @isaacs
Close: #3655
Reviewed-by: @nlf
wraithgar and others added 14 commits September 23, 2021 13:20
will filter out a small subset of non-URL-safe characters that still
parse properly with `new URL`

PR-URL: #3804
Credit: @isaacs
Close: #3804
Reviewed-by: @wraithgar
closes: #2556
xref: #1750

The xref'ed PR apparently dropped this behavior
without any explanation.

PR-URL: #3799
Credit: @gfyoung
Close: #3799
Reviewed-by: @wraithgar
  * fix: avoid infinite loops in peer dep replacements
  * fix: use Intl.Collator for string sorting when available
  * feat(vuln): expose isDirect
The npm/cli form of npm/arborist#324

Required adding options support to package used for this.

PR-URL: #3809
Credit: @isaacs
Close: #3809
Reviewed-by: @wraithgar
wraithgar and others added 8 commits September 30, 2021 08:48
Adds a new config item that includes the workspace root when running
non-arborist commands (i.e. repo, version, publis).

This also changes --workspaces to a trinary, so that setting it to false
will explicitly exclude workspaces altogether.  This is also going to
require an arborist change so that it ignores workspaces altogether.
Co-authored-by: isaacs <i@izs.me>
Co-authored-by: isaacs <i@izs.me>
@wraithgar wraithgar added Release 8.x work is associated with a specific npm 8 release and removed Release 7.x work is associated with a specific npm 7 release labels Oct 7, 2021
@wraithgar
Copy link
Member Author

This was implemented in #3890

@wraithgar wraithgar closed this Nov 2, 2021
@wraithgar wraithgar deleted the gar/workspace-dot branch November 2, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet