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

WIP: feat(workspaces): update configs #2836

Closed
wants to merge 1 commit into from

Conversation

ruyadorno
Copy link
Contributor

Add workspaces-related configs:

  • workspace: list of workspaces names/dir to filter for
  • workspaces: boolean value to enable/disable workspaces awareness

This also adds the proposed note in the docs of each of the commands
that are not affected by these configs.

References

Relates to: npm/rfcs#117

@ruyadorno ruyadorno requested a review from a team as a code owner March 7, 2021 17:17
@ruyadorno ruyadorno added Needs Review Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature labels Mar 7, 2021
Add workspaces-related configs:
  - workspace: list of workspaces names/dir to filter for
  - workspaces: boolean value to enable/disable workspaces awareness

This also adds the proposed note in the docs of each of the commands
that are not affected by these configs.

Relates to: npm/rfcs#117
@@ -12,6 +12,8 @@ npm adduser [--registry=url] [--scope=@orgname] [--always-auth] [--auth-type=leg
aliases: login, add-user
```

Note: This command is unaware of workspaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe instead of just "unaware", commands like this should be marked to indicate that it wouldn't make sense for them to be workspace-aware?

(altho in this case i can imagine wanting to log in to specific users only in specific workspaces' .npmrc files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe instead of just "unaware", commands like this should be marked to indicate that it wouldn't make sense for them to be workspace-aware?

this is me marking them to indicate that it wouldn't make sense for them to be workspace-aware 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these do make sense for it, though :-)

@@ -10,6 +10,8 @@ description: Deprecate a version of a package
npm deprecate <pkg>[@<version range>] <message>
```

Note: This command is unaware of workspaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

this definitely should be workspace-aware, since folks will want to deprecate individual (or multiple) workspaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok yeah, this comment is a clue on how we can improve the message... the true purpose of this note is to let users aware that there's no point trying to run this command with a --workspaces or --workspace=<name> configs 🤔

Copy link
Contributor Author

@ruyadorno ruyadorno Mar 15, 2021

Choose a reason for hiding this comment

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

of course folks will be able to deprecate a package even if it's managed as a workspace somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

but i mean, i might want to deprecate N packages at a time, based on workspace config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def a valid usecase 🤔 honestly I think the confusion factor here is due to the current syntax of npm deprecate that doesn't read any info from the current dir package.json file, it only accepts a single @ argument along with a message, which makes it harder to scale to multiple pkgs or workspaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that said, it sounds more like something we're going to have to redesign later and probably in a breaking change way, so I guess for now it will still have to be marked here as a command that is unaware of the workspaces configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened a RFC to follow through: npm/rfcs#341

thanks @ljharb 👍

@@ -10,6 +10,8 @@ description: Log out of the registry
npm logout [--registry=<url>] [--scope=<@scope>]
```

Note: This command is unaware of workspaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as adduser (which, since this is called "logout", should really have "login" as the canonical name for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... for now there are no support to reading .npmrc files within workspaces

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems like something worth doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or at least considered, might be waaay more work than it sounds though and probably rfc-worthy

docs/content/commands/npm-owner.md Show resolved Hide resolved
docs/content/commands/npm-whoami.md Show resolved Hide resolved
@darcyclarke darcyclarke changed the title add workspaces configs WIP: add workspaces configs Mar 11, 2021
@darcyclarke darcyclarke changed the title WIP: add workspaces configs WIP: feat(workspaces): update configs Mar 11, 2021
@ruyadorno
Copy link
Contributor Author

closing in favor of #2864

@ruyadorno ruyadorno closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants