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: use workspace spec alias by default in pnpm add #4947

Conversation

javier-garcia-meteologica
Copy link
Contributor

Makes workspace spec aliases (workspace:^, workspace:~ and workspace:*) the default version specifiers when a workspace package is added.

Related to #4887

@zkochan
Copy link
Member

zkochan commented Jun 29, 2022

Isn't it a breaking change? You suggested using a setting in #4887. Maybe it is fine to do, I am not sure. I need to test first whether changesets support it well out of the box.

@javier-garcia-meteologica
Copy link
Contributor Author

Yes, it's a breaking change.

My intention was twofold: change the default and make it configurable. I've only done the first so far because I was not able to code the configuration part.

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the configurable_pnpm_add_workspace_version branch 2 times, most recently from 30f10a5 to 7a8c96d Compare July 1, 2022 10:06
@javier-garcia-meteologica
Copy link
Contributor Author

javier-garcia-meteologica commented Jul 1, 2022

I understand that maintaining backwards compatiblity and giving users the configuration choices are important for the success of pnpm. That's why I've come up with an alternative proposal, tell me what you think.

Right now this PR makes pnpm add <workspace-pkg> to always use rolling versions (e.g. workspace:^). This is because updateProjectManifest.ts#resolvedDirectDepToSpecObject has

rolling: shouldUseWorkspaceProtocol

The change is that now I use reuse the semantics of existing options save-prefix and save-workspace-protocol. And I expand the types allowed for saveWorkspaceProtocol to saveWorkspaceProtocol: boolean | 'rolling'.

rolling: shouldUseWorkspaceProtocol && opts.saveWorkspaceProtocol === 'rolling'

We would be able to offer users the following configurability space for pnpm add <workspace-pkg>

save-prefix   save-workspace-protocol   package.json#dependencies
  ???           ???                       *  (not-reachable)
  ''            false                     1.0.0
  '^'           false                     ^1.0.0
  '~'           false                     ~1.0.0
  ''            true                      workspace:1.0.0
  '^'           true                      workspace:^1.0.0
  '~'           true                      workspace:~1.0.0
  ''            'rolling'                 workspace:*
  '^'           'rolling'                 workspace:^
  '~'           'rolling'                 workspace:~

The cons with respect to the original proposal are minimal

  • If 'rolling' becomes the new default, how would users be able to configure regular workspace specs such that workspace:^1.0.0?
  • Unable to configure pnpm such that pnpm add <workspace-pkg> produces "<workspace-pkg>": "*". But that wasn't possible before either.
  • It's not possible to specify different prefixes for regular packages and workspace ones (lack of specific save-workspace-prefix), but could be added later in a separate PR if someone ever needs it.

The pros are

  • Minimizes API surface because it reuses semantics of existing options (save-prefix, save-workspace-protocol)
  • Backwards compatibility is preserved

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the configurable_pnpm_add_workspace_version branch 2 times, most recently from 4bc0d69 to 54aa60f Compare July 1, 2022 13:57
@zkochan
Copy link
Member

zkochan commented Jul 1, 2022

If 'rolling' becomes the new default, how would users be able to configure regular workspace specs such that workspace:^1.0.0?

With save-prefix=^ and save-workspace-protocol=true, no?

Unable to configure pnpm such that pnpm add produces "": "*". But that wasn't possible before either.

That is fine.

It's not possible to specify different prefixes for regular packages and workspace ones (lack of specific save-workspace-prefix), but could be added later in a separate PR if someone ever needs it.

Sounds fine.

switch (pinnedVersion ?? 'major') {
case 'none':
return '*'
case 'major':
return `^${version}`
if (rolling) return '^'
return !version ? '*' : `^${version}`
Copy link
Member

Choose a reason for hiding this comment

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

why is this line not just ^${version}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because version may not be defined, in which case * should be used instead (assuming that rolling is not enabled). E.g. createVersionSpec(undefined, { pinnedVersion: 'major' })

Previously there was a guard at the beginning of the function to handle this case.

export function createVersionSpec (version, ...) {
  if (!version) return '*'

  ...
}

But after introducing rolling, we first have to check if rolling was requested because now we can return a pinned version even if we do not know the current version number, e.g. ^. Otherwise we check if version is defined and return a result accordingly.

packages/manifest-utils/src/getPref.ts Outdated Show resolved Hide resolved
@javier-garcia-meteologica javier-garcia-meteologica force-pushed the configurable_pnpm_add_workspace_version branch from 54aa60f to 705f561 Compare July 2, 2022 09:57
@zkochan
Copy link
Member

zkochan commented Jul 2, 2022

Please add a changeset for "pnpm" and some explanatory description with more details that you want to appear on the release page.

@zkochan zkochan added this to the v7.5 milestone Jul 2, 2022
@javier-garcia-meteologica javier-garcia-meteologica force-pushed the configurable_pnpm_add_workspace_version branch from 705f561 to 0437bca Compare July 2, 2022 10:35
@javier-garcia-meteologica
Copy link
Contributor Author

If 'rolling' becomes the new default, how would users be able to configure regular workspace specs such that workspace:^1.0.0?

With save-prefix=^ and save-workspace-protocol=true, no?

Yes, I just wanted to make sure that it was intuitive enough. I was afraid that setting a property to true in order to disable a feature with a similar name could be confusing.

Please add a changeset for "pnpm" and some explanatory description with more details that you want to appear on the release page.

Done

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.

None yet

2 participants