feat: Add pnpm support for turbo prune #1819

chris-olszewski · 2022-08-31T16:59:10Z

This PR adds pnpm lockfile support which in turn allows us to produce a pruned lockfile. Specifically it allows us the ability to decode and encode pnpm lockfiles and will build up a list of all transitive dependencies for each workspace package using the same mechanism we use for yarn.

A few things to note:

This PR only adds support for lockfile versions 5.3 and 5.4. These two give us support for pnpm 6.00-7.95 (latest). If the need arises supporting older versions is doable with some effort.
Needed to add a WorkspacePath field to the package manager abstraction since we'll need to copy this into the output directory for pnpm commands to work as expected. This wasn't needed for yarn as workspaces are defined in package.json
In the future this could also be used to be smarter about cache invalidation for pnpm projects.

Testing:
Added pnpm 6 & 7 to the list of package managers we test turbo prune against in our e2e tests.
Used turbo prune --scope docs on the example project after adding lodash to the web project. Verified that lodash wasn't in the pruned lockfile and that running pnpm install -r --frozen-lockfile ran successfully from the output dir.

vercel · 2022-08-31T16:59:15Z

@chris-olszewski is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

chris-olszewski · 2022-08-31T20:56:42Z

Looks like I need to fixup handling of CRLF for windows 🙃 . Leaving it up for review as this is a contained issue that shouldn't require any large changes to the overall code.

nathanhammond

Early comment, not a complete review.

nathanhammond · 2022-09-01T10:27:05Z

cli/internal/packagemanager/pnpm.go

@@ -28,7 +29,8 @@ var nodejsPnpm = PackageManager{
 	// We are allowed to use nil here because ArgSeparator already has a type, so it's a typed nil,
 	// This could just as easily be []string{}, but the style guide says to prefer
 	// nil for empty slices.
-	ArgSeparator: nil,
+	ArgSeparator:  nil,
+	WorkspacePath: "pnpm-workspace.yaml",


WorkspacePath sounds like an individual workspace thing.

How about something that would match this regex?
Workspace(Definition|Configuration)(Path|File)

nathanhammond · 2022-09-01T10:38:17Z

cli/internal/lockfile/pnpm_lockfile.go

+const pnpmLockfileTemplate = `lockfileVersion: {{ .Version }}
+
+importers:
+{{ range $key, $val := .Importers }}
+  {{ $key }}:
+{{ displayProjectSnapshot $val }}
+{{ end }}
+packages:
+{{ range $key, $val :=  .Packages }}
+  {{ $key }}:
+{{ displayPackageSnapshot $val }}
+{{ end }}{{ if (eq .Version 5.4) }}
+{{ end }}`


A pnpm lockfile is 100% yaml. Is there a particular reason you construct it via a template instead of going all the way to yaml and then serializing it?

The go yaml package explicitly doesn't support controlling whitespace: go-yaml/yaml#627
Having encoding/decoding to preserve the file was really helpful for development when diffing the original and pruned lockfile. If sacrificing that for a straightforward yaml serialization is desired we can do that. I'll quick double check that pnpm doesn't throw if parsing these formatting differences.

If pnpm can support arbitrary whitespace differences this may be a feature, not a bug. Non-meaningful whitespace inclusion would unnecessarily break a hash.

nathanhammond · 2022-09-01T10:42:12Z

cli/internal/lockfile/pnpm_lockfile.go

+	if p.hasCRLF {
+		var b bytes.Buffer
+		if err := t.Execute(&b, p); err != nil {
+			return err
+		}
+		scanner := bufio.NewScanner(&b)
+		crlf := []byte("\r\n")
+		for scanner.Scan() {
+			line := scanner.Bytes()
+			if _, err := w.Write(line); err != nil {
+				return errors.Wrap(err, "error copying lockfile line")
+			}
+			if _, err := w.Write(crlf); err != nil {
+				return errors.Wrap(err, "error copying lockfile line")
+			}
+		}
+		if err := scanner.Err(); err != nil {
+			return errors.Wrap(err, "error reading lockfile line from buffer")
+		}
+
+		return nil
+	}


Yowch! I really, really hope this doesn't have to stick around. /me yells at Windows.

I really dislike this as well. Due to the template usage this ended up being the least 🤮 of the solutions I came up with.

I quite like to use https://github.com/andybalholm/crlf as a wrapper for the CRLF issues :)

Thanks for that suggestion, that's so much nicer!

nathanhammond

Can we do something silly like attempt to use pnpm's lockfile in tests, and dynamically load it so we're always testing against HEAD?

https://github.com/pnpm/pnpm/blob/e6ee26e9c7bcc58b22721a182970cf029a20203a/pnpm-lock.yaml#L1

Note that upstream's version is currently 5.4-inlineSpecifiers so we're likely to get out of date quickly here.

There are some changes requested below, but basically all naming, and one where you got trolled by pnpm's dev cycle.

nathanhammond · 2022-09-06T01:45:23Z

cli/scripts/e2e/e2e.ts

+    if (["pnpm6", "pnpm"].includes(npmClient)) {
+      installArgs.push("-r");
+    }


Is this truly required? I'm not sure if the pruned state makes a difference; but install should be recursive automatically.

Ah, you're correct. Dropping the -r

cli/internal/lockfile/pnpm_lockfile.go

nathanhammond · 2022-09-06T01:53:18Z

cli/internal/lockfile/pnpm_lockfile.go

+
+// PnpmLockfile Go representation of the contents of 'pnpm-lock.yaml'
+// Reference https://github.com/pnpm/pnpm/blob/main/packages/lockfile-types/src/index.ts
+type PnpmLockfile struct {


Upstream now has Time. 😅

pnpm/pnpm@0373af2

nathanhammond · 2022-09-06T01:55:23Z

cli/internal/lockfile/pnpm_lockfile.go

+
+// PnpmLockfile Go representation of the contents of 'pnpm-lock.yaml'
+// Reference https://github.com/pnpm/pnpm/blob/main/packages/lockfile-types/src/index.ts
+type PnpmLockfile struct {


I know it's verbose but I'd prefer to match the upstream key names on all of these so that we don't have any confusion.

nathanhammond · 2022-09-06T01:58:46Z

cli/internal/lockfile/pnpm_lockfile.go

+	Resolution PackageResolution `yaml:"resolution,flow"`
+	Engines    struct {
+		Node string `yaml:"node"`
+		NPM  string `yaml:"npm,omitempty"`


Why do we need npm here? It's not in the upstream; did it cause failures? (Is it a 6/7 thing?)

It was supported in pnpm6 🙃

nathanhammond · 2022-09-06T02:05:33Z

cli/internal/lockfile/pnpm_lockfile.go

+}
+
+func isSupportedVersion(version float32) error {
+	supportedVersions := []float32{5.3, 5.4}


We should make these strings.

nathanhammond · 2022-09-06T02:06:53Z

cli/internal/lockfile/pnpm_lockfile.go

+	for _, supportedVersion := range supportedVersions {
+		if version == supportedVersion {
+			return nil
+		}
+	}


I still can't believe that this is the recommended method for testing existence in a slice.

As someone who is new to Go, this was a shock.

nathanhammond · 2022-09-06T02:08:27Z

cli/internal/lockfile/pnpm_lockfile.go

+type DependencyMeta struct {
+	Injected bool   `yaml:"injected,omitempty"`
+	Node     string `yaml:"node,omitempty"`
+	Patch    string `yaml:"string,omitempty"`
+}


I was trying to follow along on where this came from; can you add a comment here? (I still haven't figured it out.)

DependenciesMeta is the upstream name so I couldn't find it. Can we rename? (Found it.)

nathanhammond · 2022-09-06T02:11:43Z

cli/internal/lockfile/pnpm_lockfile.go

+type DependencyMeta struct {
+	Injected bool   `yaml:"injected,omitempty"`
+	Node     string `yaml:"node,omitempty"`
+	Patch    string `yaml:"string,omitempty"`


Suggested change

Patch string `yaml:"string,omitempty"`

Patch string `yaml:"patch,omitempty"`

nathanhammond · 2022-09-06T02:17:22Z

cli/internal/lockfile/pnpm_lockfile.go

+	for name, version := range entry.Dependencies {
+		deps[name] = version
+	}
+
+	for name, version := range entry.OptionalDependencies {
+		deps[name] = version
+	}
+
+	for name, version := range entry.PeerDependencies {
+		deps[name] = version
+	}


Can these collide? Do we care if they do?

From reading through pnpm I don't think these can collide. If a dep is marked optional then it won't appear in dependencies, peer deps are handled in the same manner. In the case that there's an optional peer it appears under peer with the optional attribute set true in peerDependenciesMeta

Thanks for the explanation!

Change so we build up the lockfile in memory as opposed to writing/reading an intermediate to disk. This should allow for better testing of the lockfile marshalling logic

Did a quick test of prune to make sure pnpm is still happy with the pruned lockfile: ``` olszewski@chriss-mbp pnpm-prune % turbo_dev prune --scope=docs Generating pruned monorepo for docs in /private/tmp/pnpm-prune/out - Added docs - Added ui - Added tsconfig - Added eslint-config-custom olszewski@chriss-mbp pnpm-prune % cd out olszewski@chriss-mbp out % pnpm install -r --frozen-lockfile Scope: all 5 workspace projects Lockfile is up-to-date, resolution step is skipped . | +300 ++++++++++++++++++++++++++++++ Packages are hard linked from the content-addressable store to the virtual store. Content-addressable store is at: /Users/olszewski/Library/pnpm/store/v3 Virtual store is at: node_modules/.pnpm Progress: resolved 300, reused 300, downloaded 0, added 300, done olszewski@chriss-mbp out % pnpm turbo run dev --filter=docs • Packages in scope: docs • Running dev in 1 packages docs:dev: cache bypass, force executing f63ed34e7a4cd20c docs:dev: docs:dev: > docs@0.0.0 dev /private/tmp/pnpm-prune/out/apps/docs docs:dev: > next dev --port 3001 docs:dev: docs:dev: ready - started server on 0.0.0.0:3001, url: http://localhost:3001 docs:dev: info - automatically enabled Fast Refresh for 1 custom loader docs:dev: event - compiled client and server successfully in 606 ms (154 modules) ```

chris-olszewski force-pushed the add_pnpm_lockfile_impl branch from 7907819 to 6c10f00 Compare August 31, 2022 18:01

chris-olszewski marked this pull request as ready for review August 31, 2022 18:24

chris-olszewski requested a review from a team as a code owner August 31, 2022 18:24

chris-olszewski requested review from mehulkar and gaspar09 August 31, 2022 18:24

chris-olszewski force-pushed the add_pnpm_lockfile_impl branch from cbcc0fb to 591a17c Compare August 31, 2022 22:37

nathanhammond reviewed Sep 1, 2022

View reviewed changes

chris-olszewski force-pushed the add_pnpm_lockfile_impl branch from 591a17c to 09c672c Compare September 2, 2022 18:36

nathanhammond approved these changes Sep 6, 2022

View reviewed changes

chris-olszewski added 9 commits September 8, 2022 10:18

Refactor pruned yarn lockfile writing

2587038

Change so we build up the lockfile in memory as opposed to writing/reading an intermediate to disk. This should allow for better testing of the lockfile marshalling logic

Move lockfile reading operation into package manager abstraction

49aa0c3

fixup lints

5180af6

Add pnpm lockfile parsing and writing

f787c24

Hookup pnpm lockfile to pnpm package managers

1978cb7

rebase and add e2e tests for pnpm prune

b55d94b

support windows newlines

5288cb9

fixups for PR

677bcb6

chris-olszewski force-pushed the add_pnpm_lockfile_impl branch from 09c672c to 677bcb6 Compare September 8, 2022 18:49

chris-olszewski added 2 commits September 8, 2022 11:56

Merge branch 'main' into add_pnpm_lockfile_impl

489fa24

Merge branch 'main' into add_pnpm_lockfile_impl

1385938

chris-olszewski merged commit 8645530 into vercel:main Sep 8, 2022

chris-olszewski mentioned this pull request Sep 9, 2022

Implementing turbo prune command for pnpm workspaces #534

Closed

zigang93 mentioned this pull request Sep 13, 2022

Pnpm workspace still unable to prune #1941

Closed

MorZeevi mentioned this pull request Sep 15, 2022

Don't bust cache on new workspace packages #1304

Closed

	Patch string `yaml:"string,omitempty"`
	Patch string `yaml:"patch,omitempty"`

feat: Add pnpm support for turbo prune #1819

feat: Add pnpm support for turbo prune #1819

Uh oh!

Conversation

chris-olszewski commented Aug 31, 2022

Uh oh!

vercel bot commented Aug 31, 2022

Uh oh!

chris-olszewski commented Aug 31, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nathanhammond left a comment •

edited

Loading