-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[v13] Use pnpm for package management #11799
Conversation
If we are going to use pnpm workspaces we should remove all the other stuff like |
0439b79
to
248942c
Compare
A whole bunch of these commits could be shipped independently if we'd like to reduce the size of this PR. They're fixing issues that only become apparent after pnpm's change to no longer hoist transitive & sub-package dependencies, and would work without pnpm |
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.
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.
Approved! With one ask to revert some of the code formatting changes (just to keep the diff size down).
@@ -16,7 +16,7 @@ import styles from './ThemeProvider.module.css'; | |||
*/ | |||
export const themeNamesLocal = ['light', 'dark-experimental'] as const; | |||
|
|||
type ThemeNameLocal = typeof themeNamesLocal[number]; | |||
type ThemeNameLocal = (typeof themeNamesLocal)[number]; |
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.
Bunch of these appear to be prettier-related changes.
Do you mind if we revert these and bring them in a separate PR (they're functionally equivalent)?
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.
Works for me!
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.
Extracted here: #11824
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.
Interesting, CI fails there: https://github.com/Shopify/polaris/actions/runs/8528174905/job/23361209192?pr=11824
So it would seem that we're ending up with a different version of a key package (TypeScript? Eslint?) when linting one of these packages if using yarn instead of pnpm
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.
Ah damn, in that case, leave the changes in this PR. Sorry for the extra work there!
I hacked together a script (below) to try compare the build output with pnpm expecting zero changes (it's the package manager that's changed, not the packages themselves), but... There's quite a bit of difference 😓 Some of it seems to be slight differences in however the build tool has generated the final JS with extra parens here and there, but then there's other parts which appear to change the exports of files, etc 😱 Give it a run yourself and see if you're noticing similar differences: Put this in the root of the Polaris repo as #!/usr/bin/env bash
declare -a build_folders=(
"polaris-for-vscode/dist/"
"polaris-icons/dist/"
"polaris-migrator/dist/"
"polaris-react/build/"
"polaris-tokens/dist/"
"polaris.shopify.com/.next/"
"polaris.shopify.com/public/playroom/"
"polaris.shopify.com/public/og-images/"
"polaris.shopify.com/public/og-images/"
)
git fetch
source ~/.nvm/nvm.sh
MAIN_COMMIT=`git merge-base origin/pnpm origin/main`
# # Get a clean repo at the latest shared ancestor that's using yarn
git worktree add ../polaris-main $MAIN_COMMIT
cd ../polaris-main
nvm use
yarn
yarn build
for i in "${build_folders[@]}"
do
mkdir -p "../polaris-build/main/""$i"
cp -R "$i" "../polaris-build/main/""$i"
done
cd -
# Cleanup
git worktree remove -f ../polaris-main
git worktree add ../polaris-pnpm origin/pnpm
cd ../polaris-pnpm
nvm use
pnpm install
pnpm build
for i in "${build_folders[@]}"
do
mkdir -p "../polaris-build/pnpm/""$i"
cp -R "$i" "../polaris-build/pnpm/""$i"
done
cd -
# Cleanup
git worktree remove -f ../polaris-pnpm
echo "Now run: git diff --no-index ../polaris-build/main ../polaris-build/pnpm" |
Thanks for putting this together @jesstelford ! It looks to me like a lot of changes to the nextJS compiled output (shouldn't be a big deal for our purposes) and also some changes to the way the dist files get build & minified. This suggests to me that perhaps the version of Rollup or one of its sub-dependencies has changed. Maybe a version that one of the packages depends on was previously improperly hoisted? It'd be great to try testing the bundles in some of our consumers like |
/snapit |
@sam-b-rose after removing the const packages = require('./package.json').workspaces.packages; If we want a 1-1 replacement, I can add a dependency on |
@ryanwilsonperkin +1 to continuing with |
### WHY are these changes introduced? This package depedends on React as a peer dependency in its produced output for SVG icons that end up rended as React components. Extracted from #11799 which highlighted the issue <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. Include a video if your changes include interactive content. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### 🎩 checklist - [ ] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide --------- Co-authored-by: Sam Rose <[email protected]>
/snapit |
@ryanwilsonperkin We're prepping for a major release of Polaris in the next week or so (to update the minimum node version) - how do you feel about this change being merged as part of the major release? I'm happy with the change, but given the changes seen in that script I ran, I wanted to just play it safe and push it out as a major in case something has changed. ☝️ Moderate opinion, weakly held |
@jesstelford I like that idea as it's a pretty significant change for developers contributing to Polaris. |
Ok, in that case I'll mark this PR as Draft until we're ready for the v13 release. @ryanwilsonperkin are you ok if myself & @aaronccasanova take over this PR from here? We'll shepherd it into the wild ASAP 🎉 |
Sounds good to me! Thanks @jesstelford |
291ab0b
to
25105fa
Compare
Heads up; I've rebased this on top of the |
927f3d9
to
2b0863e
Compare
package.json
Outdated
"new-migration": "yarn workspace @shopify/polaris-migrator generate", | ||
"postinstall": "patch-package" | ||
"release-packages": "pnpm build:release && changeset publish", | ||
"new-migration": "pnpm workspace @shopify/polaris-migrator generate" |
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.
"new-migration": "pnpm workspace @shopify/polaris-migrator generate" | |
"new-migration": "pnpm --filter @shopify/polaris-migrator generate" |
@@ -2,7 +2,7 @@ | |||
|
|||
/* Looking for media queries? They're imported in the postcss config in | |||
polaris-react/config/postcss-plugins.js */ | |||
@import '../../../../node_modules/@shopify/polaris-tokens/dist/css/styles.css'; | |||
@import '../../../node_modules/@shopify/polaris-tokens/dist/css/styles.css'; |
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.
yarn
used to hoist everything to the root node_modules
, but pnpm
doesn't hoist like that, so we only need to go up to the polaris-react/node_modules
to find the dependency.
🚀🚀🚀 |
Edited by @jesstelford: Because speed.