-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Add pnpm integration test #17882
Merged
Merged
Add pnpm integration test #17882
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
531c55a
Upgrade pnpm to latest
elliottsj 8728a7b
Add pnpm integration test
elliottsj 3747ebf
Replace 'basic-pnpm' test
elliottsj 840a8cb
Ensure "next"'s monorepo dependencies are installed
elliottsj 0e8d649
Update pnpm to latest
elliottsj 23e6431
Replace pnpmfile.js with "pnpm.overrides"
elliottsj 6be3048
Replace `Object.fromEntries` with for loop
elliottsj 8fe0664
Merge branch 'canary' into pnpm-integration-test
elliottsj 94e9840
Merge upstream/canary into pnpm-integration-test
elliottsj 510ed40
Upgrade to pnpm@5.14.3
elliottsj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Ensure "next"'s monorepo dependencies are installed
- Loading branch information
commit 840a8cbffb07eb9fc139adaf313438e1151529ff
There are no files selected for viewing
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, does this cover bundling the dependencies needed like
@next/react-dev-overlay
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. Just checked and it seems that only the "next" package is packed, not the other monorepo dependencies like
@next/polyfill-module
,@next/react-dev-overlay
.I pushed an change which packs
@next/env
,@next/polyfill-module
,@next/react-dev-overlay
, and@next/react-refresh-utils
into tarballs too, and ensures they're installed upon runningpnpm install
by using a pnpmfile.js hook.This is the best I could come up with to make the install process as similar as possible to the real world (installing from a registry), without actually installing from a registry.
An even more correct pnpm test would involve publishing all of the monorepo packages to a local npm registry (maybe verdaccio?), then telling pnpm to install packages from there. Would something like this be preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm has a new package.json overrides feature in v5.10.1, so I upgraded and switched to that. It's a bit cleaner now, since we don't have to generate an executable pnpmfile.js file anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 💯 Will re-review this PR soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timneutkens thanks. Can't wait for this.