Switch from NPM to PNPM for package management #1818
Conversation
Advantage of PNPM is speed and storage size on disk: if you have multiple projects using the same NPM package, NPM will store multiple copies, but PNPM will store only one copy and share it between projects. Most recent version of PNPM also requires Node version 18 or later, so we'll bump any Node 16 references to most recent Node (22.2.0).
Also lets us remove the rebuild of node-sass, which was needed for devs running a different architecture on their dev machines than the architecture the Docker containers are built for (e.g. devs running Macs with ARM chips, when the Docker containers we build are for x64).
Version 2 of date-fns had some assumptions built into the package about the layout of node_modules that aren't true when pnpm is managing it. Version 3 of date-fns fixed those assumptions.
rmunn
added
the
engineering
Unit Test Results362 tests 362 ✅ 12s ⏱️ Results for commit f9f550d. ♻️ This comment has been updated with latest results. |
By specifying `target: development` in docekr-compose.yml, we get the same effect as specifying `docker build --target=development`: with buildx, unused build stages (e.g., prod stages) are skipped.
| run: npm ci | ||
| name: Prep for production build of legacy app | ||
| # docker compose build doesn't allow passing a `--target` parameter, so we have to replace the target in docker-compose.yml | ||
| run: "sed -i 's/target: development/target: production/g' docker-compose.yml" |
There was a problem hiding this comment.
instead of this could we introduce a merge and a prod build yml file? That way if we need to make other tweaks for prod we can do it easily via adding changes to the file?
then our compose might look like this docker compose -f compose.yml -f compose.prod.yml up -d where the prod just contains the different targets we want, very similar to kustomzie actually.
There was a problem hiding this comment.
+1 for the compose merge solution. That could also be a separate PR?
There was a problem hiding this comment.
I'll make it a separate PR to minimize the chances of merge conflicts (e.g., #1820 also touches workflows and I expect one merge conflict there, which will be easily resolved but if I'm making larger changes to the build process I might get harder-to-handle conflicts).
There was a problem hiding this comment.
Resolving conversation for now, just to unlock the merge button. Then I'll come back and unresolve it so that the suggestion is easy for me to find later.
| run: npm ci | ||
| name: Prep for production build of legacy app | ||
| # docker compose build doesn't allow passing a `--target` parameter, so we have to replace the target in docker-compose.yml | ||
| run: "sed -i 's/target: development/target: production/g' docker-compose.yml" |
There was a problem hiding this comment.
+1 for the compose merge solution. That could also be a separate PR?
One case switched to pnpm dlx instead of pnpm exec because there's no package.json or node_modules in the test/e2e folder so pnpm exec would fail, but pnpm dlx will run the tool globally regardless of location.
Can't use `pnpm exec` here either since it's being run in a step where `pnpm i` has *not* happened, so there's no prettier binary to run.
Fixed issues
Fixes #1811.
Fixes #1653.
Fixes #1654.
Description
Switching to PNPM has a couple of advantages. The first is speed: pnpm install is typically faster than npm install, though how much will depend heavily the speed of your Internet connection. The second is disk usage: if you have multiple copies of the Language Forge repository on your disk, perhaps because you want to keep a copy of
developrunning while you work on a different branch in another folder, PNPM will de-duplicate the packages whose versions are the same between the copies (which will be 100% of them unless you're fiddling withpackage.jsonin your branch).Screenshots
No visible changes.
Checklist
Testing
Testers, use the following instructions against our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.
Describe how to verify your changes and provide any necessary test data.
makecorepack enablethen runmakeagain.