[WB-1870] Adopt pnpm in Wonder Blocks #2453
Conversation
🦋 Changeset detectedLatest commit: ebc3f2a The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-bbxjfzdwew.chromatic.com/ Chromatic results:
|
jandrade
changed the title
…ng rollup to v4 and @babel/runtime bump to match the current webapp version.
.changeset/polite-pianos-serve.md
Outdated
There was a problem hiding this comment.
note: I've marked this as minor as the bundled ES files didn't change too much. I only used minor mostly b/c rollup is upgrading from v2 to v4, but there shouldn't be breaking changes in WB packages.
.eslintrc.js
Outdated
There was a problem hiding this comment.
note: I had to modify some rules to account for some package updates, but it mostly affect tests, not prod code.
There was a problem hiding this comment.
note: I created this custom action based on the one @jeresig created in the main repo (see ./github/actions/shared-node-cache/action.yml).
A bunch of files are now using this internal action instead of the external one to avoid breaking other repos that still rely on yarn.
|
|
||
| - name: Verify changeset entries | ||
| uses: Khan/[email protected] | ||
| - uses: Khan/actions@check-for-changeset-v1 |
There was a problem hiding this comment.
note: Reverted back to use Khan/actions@check-for-changeset-v1 as it doesn't rely on a package manager to resolve dependency changes.
The reason why Khan/changeset-per-package is no longer viable with pnpm is that this action relies heavily on yarn: https://github.com/Khan/changeset-per-package/blob/main/src/main.ts#L35
One thing to take into consideration is that the WB repo never got issues with check-for-changeset, so reverting back to it should be safe.
.storybook/.babelrc.js
Outdated
There was a problem hiding this comment.
note: Updated some packages, and that needed to update the plugin references in here.
| async viteFinal(config, options) { | ||
| // Merge custom configuration into the default config | ||
| const {mergeConfig} = await import("vite"); | ||
|
|
||
| return mergeConfig(config, { | ||
| // Prevent Vite from inlining phosphor-icons | ||
| build: { | ||
| assetsInlineLimit: 0, | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
note: I tried to define assetsInlineLimit directly in vite.config.ts but Storybook seems to have an issue reading that file, so I had to move that part here to avoid having issues with SVG rendering in Storybook.
| @@ -464,7 +464,7 @@ export const SingleSection: StoryComponentType = { | |||
| return ( | |||
| <Accordion> | |||
| {[ | |||
| <AccordionSection header="First section"> | |||
| <AccordionSection header="First section" key={0}> | |||
There was a problem hiding this comment.
note: Added key in a bunch of story examples to prevent a browser warning where a collection of nodes need a specific key/id in React.
| @@ -41,7 +41,7 @@ const createConfig = (pkgName) => { | |||
| presets, | |||
| plugins, | |||
| exclude: "node_modules/**", | |||
| runtimeHelpers: true, | |||
| babelHelpers: "runtime", | |||
There was a problem hiding this comment.
note: This API changed with rollup v4, so I just had to make it compatible.
| "typecheck": "tsc", | ||
| "typewatch": "tsc --noEmit --watch --incremental", | ||
| "add:devdepbysha": "bash -c 'yarn add -W --dev \"git+https://[email protected]/Khan/$0.git#$1\"'" | ||
| "add:devdepbysha": "bash -c 'pnpm add -W --dev \"git+https://[email protected]/Khan/$0.git#$1\"'" | ||
| }, | ||
| "author": "", | ||
| "license": "MIT", | ||
| "devDependencies": { |
There was a problem hiding this comment.
note: Used pnpm u -i --latest to update a bunch of dev dependencies in the repo. I tried to preserve or update to some specific versions in some cases to be compatible with webapp (e.g. babel).
There was a problem hiding this comment.
Thanks for updating our dev dependencies!
| @@ -29,6 +30,6 @@ | |||
| "react": "18.2.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@khanacademy/wb-dev-build-settings": "^2.0.0" | |||
| "@khanacademy/wb-dev-build-settings": "workspace:*" | |||
There was a problem hiding this comment.
note: modified this in all WB packages to manage build-settings as a workspace alias.
https://pnpm.io/workspaces#referencing-workspace-packages-through-aliases
There was a problem hiding this comment.
Out of curiosity, is there a reason why the other wonder-blocks dependencies don't use workspace:* as well? (ex: above we have "@khanacademy/wonder-blocks-clickable": "^6.0.0", instead of workspace:* also!)
There was a problem hiding this comment.
That's an excellent question! I'm not sure why I didn't try that, but I don't see why not. My only concern would be that maybe changesets would need to find a way to figure out how to bump to the proper versions. I'll try to see what happens.
There was a problem hiding this comment.
@beaesguerra I was looking at monorepos with similar tooling and it seems that it should work (in theory). But in order to test that properly, I'm going to land this into main, so we can get the "Check build sizes" workflow step running, then I'll experiment with it in a separate PR to see if something breaks and/or the bundled versions are affected. If that works, it would be really great as we would reduce the times we'd need to update package.json files, or it would simplify such updates. I'll keep you posted about this, and thanks for the suggestion/question!
There was a problem hiding this comment.
Sounds like a good plan, thanks for looking into this!
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (7834dd9) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2453"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2453 |
| "typecheck": "tsc", | ||
| "typewatch": "tsc --noEmit --watch --incremental", | ||
| "add:devdepbysha": "bash -c 'yarn add -W --dev \"git+https://[email protected]/Khan/$0.git#$1\"'" | ||
| "add:devdepbysha": "bash -c 'pnpm add -W --dev \"git+https://[email protected]/Khan/$0.git#$1\"'" | ||
| }, | ||
| "author": "", | ||
| "license": "MIT", | ||
| "devDependencies": { |
There was a problem hiding this comment.
Thanks for updating our dev dependencies!
| @@ -29,6 +30,6 @@ | |||
| "react": "18.2.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@khanacademy/wb-dev-build-settings": "^2.0.0" | |||
| "@khanacademy/wb-dev-build-settings": "workspace:*" | |||
There was a problem hiding this comment.
Out of curiosity, is there a reason why the other wonder-blocks dependencies don't use workspace:* as well? (ex: above we have "@khanacademy/wonder-blocks-clickable": "^6.0.0", instead of workspace:* also!)
| @@ -23,6 +22,7 @@ describe("SerializableInMemoryCache", () => { | |||
|
|
|||
| it("should throw if the cloning fails", () => { | |||
| // Arrange | |||
| const WSCore = require("@khanacademy/wonder-stuff-core"); | |||
There was a problem hiding this comment.
For my own learning, how come we had to change how we were importing WSCore?
There was a problem hiding this comment.
TBH I changed devDeps and at some point those tests started to break. I don't recall changing anything related to Jest, so this was surprising to me. I applied this solution as I encountered similar situations in webapp and/or WB in the past and that solved the issue.
My guess is that pnpm is not able to import the same dependency that is used in the implementation, so the mock is never called. I switched to this approach to ensure that we always import/require the right dep. It might have to do with the fact that Jest still uses CJS for testing (where as the code uses ES Modules).
| async viteFinal(config, options) { | ||
| // Merge custom configuration into the default config | ||
| const {mergeConfig} = await import("vite"); | ||
|
|
||
| return mergeConfig(config, { | ||
| // Prevent Vite from inlining phosphor-icons | ||
| build: { | ||
| assetsInlineLimit: 0, | ||
| }, | ||
| }); | ||
| }, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2453 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Summary:
eslintasthe latest version is not compatible with our current setup.
Issue: WB-1870
Test plan:
Verify that all the checks pass and that the Storybook site works fine.