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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jul 2, 2022
Merged

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

merged 1 commit into from
Jul 2, 2022

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

@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
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 •
edited
Loading

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.

zkochan
zkochan reviewed Jul 1, 2022
View reviewed changes
packages/manifest-utils/src/getPref.ts
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 •
edited
Loading

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

@zkochan zkochan merged commit f5621a4 into pnpm:main Jul 2, 2022
This was referenced Jul 2, 2022
Open
Merged
Merged
Merged
Merged
Merged
Open
Open
Merged
Closed
Closed
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