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: ignore implict workspace for some commands #4479

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

fritzy
Copy link
Contributor

@fritzy fritzy commented Feb 28, 2022

For some commands, ignore implicit cwd workspace config and run as if no workspace filter is set.

'adduser', 'access', 'login', 'bin', 'birthday', 'bugs', 'cache', 'help-search', 'help', 'hook', 'logout', 'org', 'ping', 'prefix', 'profile', 'root', 'search', 'token'

closes #4404

Special thanks to @mshima for submitting a similar PR #4439

@fritzy fritzy requested a review from a team as a code owner February 28, 2022 05:10
@npm-robot
Copy link
Contributor

npm-robot commented Feb 28, 2022

no statistically significant performance changes detected

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 43.627 ±0.32 28.568 ±0.04 17.840 ±0.16 20.296 ±0.91 2.961 ±0.07 2.901 ±0.00 2.333 ±0.12 12.123 ±0.15 2.262 ±0.06 3.304 ±0.13
#4479 43.374 ±0.69 28.454 ±0.08 17.856 ±0.31 19.994 ±0.34 2.890 ±0.02 2.890 ±0.06 2.280 ±0.02 12.355 ±0.22 2.427 ±0.03 3.532 ±0.38
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 31.811 ±0.09 29.638 ±10.40 13.319 ±0.32 14.273 ±0.01 2.657 ±0.10 2.700 ±0.10 2.277 ±0.07 8.801 ±0.09 2.212 ±0.02 2.980 ±0.07
#4479 34.758 ±0.86 23.478 ±0.52 14.095 ±0.04 15.127 ±0.46 2.866 ±0.03 2.706 ±0.00 2.502 ±0.10 9.522 ±0.19 2.277 ±0.02 3.298 ±0.04

lib/npm.js Outdated Show resolved Hide resolved
lib/base-command.js Outdated Show resolved Hide resolved
@antongolub
Copy link

Shouldn't ignoreImplicitWorkspace be set true for whoami?

@wraithgar
Copy link
Member

Shouldn't ignoreImplicitWorkspace be set true for whoami?

What if a workspace has an alternate registry or auth config?

Comment on lines +123 to +127
// only call execWorkspaces when we have workspaces explicitly set
// or when it is implicit and not in our ignore list
const filterByWorkspaces =
(workspacesEnabled || workspacesFilters.length > 0)
&& (!implicitWorkspace || !command.ignoreImplicitWorkspace)
Copy link

@mshima mshima Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust the comment:

Suggested change
// only call execWorkspaces when we have workspaces explicitly set
// or when it is implicit and not in our ignore list
const filterByWorkspaces =
(workspacesEnabled || workspacesFilters.length > 0)
&& (!implicitWorkspace || !command.ignoreImplicitWorkspace)
// only call execWorkspaces when we have workspaces explicitly set
// and when it is not implicit or not marked as ignore implicit workspace
const filterByWorkspaces =
(workspacesEnabled || workspacesFilters.length > 0)
&& (!implicitWorkspace || !command.ignoreImplicitWorkspace)

Copy link

@mshima mshima Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And implicitWorkspace shouldn't conflict with explicit workspace? Or explicit shouldn't take precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of ways we could slice it. Right now, if your cwd is a workspace, it'll run the command and ignore any explicit workspace you specify. This makes sense for something like bin, which has always worked on cwd, but it could be confusing for other commands. If you're not in a workspace and and you specify one, many commands will still fail.

We could go back and forth on the best way to handle these edge cases, but ultimately, I'd rather ship and iterate. If there are cases and commands that need more tweaking, we could patch the behavior again. But for now, this gets us back to running commands that don't really care about workspaces, but generally still errors if you try to explicitly set a workspace on a command that doesn't support them.

lib/npm.js Outdated Show resolved Hide resolved
@wraithgar wraithgar changed the base branch from latest to release-next March 1, 2022 15:48
@wraithgar wraithgar merged commit 45fc297 into release-next Mar 2, 2022
@wraithgar wraithgar deleted the fritzy/workspace-adduser branch March 2, 2022 15:40
This was referenced Mar 3, 2022
@jedwards1211
Copy link

jedwards1211 commented Aug 30, 2024

@wraithgar

What if a workspace has an alternate registry or auth config?

What would such a config look like? I'm having trouble finding anything about it in the npm docs

And also if so, I must fundamentally misunderstand something:

  • wouldn't the alternate registry also apply to adduser, login, logout, etc?
  • why does whoami say it doesn't support workspaces? Shouldn't it just get the user associated with the workspace's registry/auth config?

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.

[BUG] npm adduser fails in workspace.
7 participants