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

[rush] Add support for PNPM version 3.x #1210

Merged
merged 17 commits into from
Apr 23, 2019
Merged

[rush] Add support for PNPM version 3.x #1210

merged 17 commits into from
Apr 23, 2019

Conversation

tnc1997
Copy link
Contributor

@tnc1997 tnc1997 commented Apr 5, 2019

This pull request follows on from issues #1145 and #1180 and adds preliminary support for pnpm version 3.

The --resolution-strategy flag has been added to the pnpmOptions section of rush.json and both of the different filenames given to the pnpm 'lock' files based on the pnpm version are supported.

@msftclas
Copy link

msftclas commented Apr 5, 2019

CLA assistant check
All CLA requirements met.

@octogonz
Copy link
Collaborator

@iclanton I tested this over the weekend, and it's in good shape, but I found that PNPM 3.0.0 fails if the new option is provided. I was working on a fix for that yesterday but got distracted. I can finish it tonight.

@octogonz octogonz changed the title [rush] Add preliminary support for pnpm version 3 [rush] Add support for PNPM version 3.x Apr 23, 2019
Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

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

AWS

apps/rush-lib/src/api/PackageManagerFeatureSet.ts Outdated Show resolved Hide resolved
export type PackageManager = 'pnpm' | 'npm' | 'yarn';

/**
* Reports the known features of a package manager as detected from its version number.
Copy link
Member

Choose a reason for hiding this comment

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

Drop the "number" at the end of this sentence.


this.supportsPnpmResolutionStrategy = false;

switch (this.packageManager) {
Copy link
Member

Choose a reason for hiding this comment

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

It kinda feels like this data should be expressed in JSON files. TS for code, JSON for data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, but let's collect a little more "data" before we design a specialized manager for it

Copy link
Member

Choose a reason for hiding this comment

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

Sure - that's fine.

apps/rush-lib/src/api/RushConfiguration.ts Show resolved Hide resolved
@octogonz octogonz merged commit 7ba42b7 into microsoft:master Apr 23, 2019
@octogonz
Copy link
Collaborator

This PR was published with Rush 5.7.0

@octogonz
Copy link
Collaborator

In PR #1244 the build is failing with this error:

ERROR: Invalid Version: 1.0.3_@pnpm+logger@1.0.2

TypeError: Invalid Version: 1.0.3_@pnpm+logger@1.0.2
    at new SemVer (C:\Users\Owner\.rush\node-v10.15.2\rush-5.7.0\node_modules\semver\semver.js:293:11)
    at Range.test (C:\Users\Owner\.rush\node-v10.15.2\rush-5.7.0\node_modules\semver\semver.js:1036:15)
    at Function.satisfies (C:\Users\Owner\.rush\node-v10.15.2\rush-5.7.0\node_modules\semver\semver.js:1085:16)
    at PnpmShrinkwrapFile.checkValidVersionRange

It seems the shrinkwrap.yaml changed from this:

'file:projects/rush-lib.tgz':
    dependencies:
      '@pnpm/link-bins': /@pnpm/link-bins/1.0.3/@pnpm!logger@1.0.2

...to this in the new pnpm-lockfile.json:

  'file:projects/rush-lib.tgz':
    dependencies:
      '@pnpm/link-bins': 1.0.3_@pnpm+logger@1.0.2

@tnc1997 @bsiegel did you encounter this? Maybe we need to update the version parsing in PnpmShrinkwrapFile.ts

@tnc1997
Copy link
Contributor Author

tnc1997 commented Apr 23, 2019

Hi @octogonz, I've just checked out all of the latest changes and I can confirm that I too am getting the same error. As you mention, it seems to point towards a change in how version numbers are represented in the latest version of pnpm, which might require a modification to the version parsing used.

@octogonz
Copy link
Collaborator

I'm going to see if I can debug it tonight

@octogonz
Copy link
Collaborator

Okay, I have a fix. I'll create a PR later tonight.

@octogonz
Copy link
Collaborator

Here's the PR: #1246

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.

5 participants