Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds pnpm as a supported package manager in the Node.js buildpack, including dependency installation, caching behavior, and integration coverage.
Changes:
- Detect pnpm usage via
pnpm-lock.yaml/pnpm-workspace.yamland supportengines.pnpminpackage.json. - Install pnpm (via npm) and run dependency builds using a new pnpm build path and cache directory (
.pnpm-store). - Add unit + integration tests and pnpm fixtures (simple app + workspace app) to validate behavior.
Reviewed changes
Copilot reviewed 26 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nodejs/supply/supply_test.go | Extends supplier unit tests for pnpm detection, engines parsing, pnpm install, caching, and build flow. |
| src/nodejs/supply/supply.go | Adds pnpm wiring to supplier (detection, install, build selection, dependency listing, cache handling). |
| src/nodejs/supply/mocks_test.go | Adds a PNPM mock to support new supplier tests. |
| src/nodejs/supply/cli/main.go | Wires the new pnpm implementation into the supply CLI. |
| src/nodejs/pnpm/pnpm.go | Introduces pnpm build implementation (pnpm config set store-dir, pnpm install). |
| src/nodejs/pnpm/pnpm_test.go | Adds unit tests for pnpm build behavior (store-dir, prod mode, offline vendored store). |
| src/nodejs/pnpm/pnpm_suite_test.go | Adds ginkgo suite for pnpm package tests. |
| src/nodejs/pnpm/mocks_test.go | Adds gomock for pnpm Command interface used by tests. |
| src/nodejs/package_json/package_json.go | Adds engines.pnpm parsing + logging. |
| src/nodejs/integration/init_test.go | Registers PNPM integration suite. |
| src/nodejs/integration/pnpm_test.go | Adds integration tests for pnpm “simple” and “workspaces” fixture deployments. |
| fixtures/pnpm/simple/package.json | Adds a pnpm-based fixture app (simple). |
| fixtures/pnpm/simple/pnpm-lock.yaml | Adds lockfile for simple pnpm fixture. |
| fixtures/pnpm/simple/server.js | Adds server for simple pnpm fixture. |
| fixtures/pnpm/workspaces/package.json | Adds a pnpm workspace fixture root package.json. |
| fixtures/pnpm/workspaces/pnpm-lock.yaml | Adds lockfile for pnpm workspace fixture. |
| fixtures/pnpm/workspaces/pnpm-workspace.yaml | Adds pnpm workspace definition. |
| fixtures/pnpm/workspaces/packages/sample-lib/package.json | Adds sample workspace library package metadata. |
| fixtures/pnpm/workspaces/packages/sample-lib/index.js | Adds sample workspace library code. |
| fixtures/pnpm/workspaces/packages/sample-app/package.json | Adds sample workspace app package metadata. |
| fixtures/pnpm/workspaces/packages/sample-app/index.js | Adds sample workspace app server code. |
| fixtures/pnpm/workspaces/packages/pkg-a/package.json | Adds workspace package A metadata. |
| fixtures/pnpm/workspaces/packages/pkg-a/server.js | Adds workspace package A server. |
| fixtures/pnpm/workspaces/packages/pkg-b/package.json | Adds workspace package B metadata. |
| fixtures/pnpm/workspaces/packages/pkg-b/index.js | Adds workspace package B code. |
| fixtures/pnpm/unmet/package.json | Adds pnpm unmet-deps fixture package.json. |
| fixtures/pnpm/unmet/pnpm-lock.yaml | Adds pnpm unmet-deps fixture lockfile. |
| fixtures/pnpm/dev_deps/package.json | Adds pnpm dev-deps fixture package.json. |
| fixtures/pnpm/dev_deps/pnpm-lock.yaml | Adds pnpm dev-deps fixture lockfile. |
| .gitignore | Ignores node_modules in the repo. |
Files not reviewed (4)
- fixtures/pnpm/dev_deps/pnpm-lock.yaml: Language not supported
- fixtures/pnpm/simple/pnpm-lock.yaml: Language not supported
- fixtures/pnpm/unmet/pnpm-lock.yaml: Language not supported
- fixtures/pnpm/workspaces/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9b7d062 to
dd7e6cc
Compare
8f1d8f5 to
1f53aaa
Compare
3cb2786 to
838214f
Compare
Author
|
Integration tests all pass now. I have no idea how to add the pnpm binary to the manifest properly, so it currently pulls from GitHub. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for contributing to the buildpack. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change: add the long overdue pnpm support
An explanation of the use cases your change solves: adds support for a package manager which speeds up the build times by being much more efficient than npm or yarn 1. Since this is a massive improvement for time spend in CI (e.g. for lifecycle tests), it may decrease expenses for the same purpose. When switching a particular MTA app to use pnpm, it effectively cut build times in half for this one use case tested. This was requested here as well, and closed without reason:
pnpmsupport #398I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the
masterbranchI have added an integration test