Skip to content

feat: use workspace spec alias by default in pnpm add#4947

Merged
zkochan merged 1 commit into
pnpm:mainfrom
javier-garcia-meteologica:configurable_pnpm_add_workspace_version
Jul 2, 2022
Merged

feat: use workspace spec alias by default in pnpm add#4947
zkochan merged 1 commit into
pnpm:mainfrom
javier-garcia-meteologica:configurable_pnpm_add_workspace_version

Conversation

@javier-garcia-meteologica

@javier-garcia-meteologica javier-garcia-meteologica commented Jun 28, 2022

Copy link
Copy Markdown
Contributor

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

Related to #4887

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the configurable_pnpm_add_workspace_version branch from cfa463a to 4964de1 Compare June 28, 2022 18:10
@zkochan

zkochan commented Jun 29, 2022

Copy link
Copy Markdown
Member

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

javier-garcia-meteologica commented Jun 30, 2022

Copy link
Copy Markdown
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

javier-garcia-meteologica commented Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

zkochan commented Jul 1, 2022

Copy link
Copy Markdown
Member

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.

zkochan
zkochan reviewed Jul 1, 2022
View reviewed changes
Comment thread packages/manifest-utils/src/getPref.ts
case 'major':
return `^${version}`
if (rolling) return '^'
return !version ? '*' : `^${version}`

@zkochan zkochan Jul 1, 2022

Copy link
Copy Markdown
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}?

@javier-garcia-meteologica javier-garcia-meteologica Jul 2, 2022

Copy link
Copy Markdown
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.

Comment thread packages/manifest-utils/src/getPref.ts Outdated
@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

zkochan commented Jul 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

javier-garcia-meteologica commented Jul 2, 2022

Copy link
Copy Markdown
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

@zkochan zkochan merged commit f5621a4 into pnpm:main Jul 2, 2022
This was referenced Jul 2, 2022
Open
Merged
Merged
Merged
Merged
Merged
Open
Merged
Closed
Closed
Merged
This was referenced Aug 5, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Closed
This was referenced Aug 12, 2022
Merged
Merged
Merged
This was referenced Aug 20, 2022
Merged
Merged
Merged
Open
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
@renovate renovate Bot mentioned this pull request Aug 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zkochan zkochan zkochan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v7.5

Development

Successfully merging this pull request may close these issues.

2 participants

@javier-garcia-meteologica @zkochan