Skip to content
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

EES-3976 - migrate to PNPM #3782

Merged
merged 1 commit into from
Jan 26, 2023
Merged

EES-3976 - migrate to PNPM #3782

merged 1 commit into from
Jan 26, 2023

Conversation

luke-h1
Copy link
Contributor

@luke-h1 luke-h1 commented Jan 18, 2023

This PR migrates the project from Lerna + NPM to PNPM & PNPM workspaces.

Getting started with PNPM

In order to set up the project & install dependencies for various packages, you would previously have to run two commands: npm ci && npm run bootstrap. With PNPM this command now simply turns into pnpm i.
pnpm i will install the root dependencies along with all the workspace packages defined in pnpm-workspace.yaml

When it comes to installing new dependencies, you simply modify the relevant package.json and run pnpm i

When it comes to running scripts you can choose to omit the run part of your command as PNPM can work just fine without it.
i.e.

# before 
npm run lint && npm run format 

# after 
pnpm lint && pnpm format 

Install PNPM & project dependencies

# install PNPM via corepack (https://github.com/nodejs/corepack)
corepack enable

# install dependencies 
pnpm i 

Useful links:

Introduction

Due to the way PNPM symlinks dependencies and the stricter structure it provides out of the box (see https://pnpm.io/symlinked-node-modules-structure), we've had to enable the shamefully-hoist option in order to create a flat, non-symlinked node_module structure. This is pretty much what the project uses at the moment which is why I've encountered minimal problems approaching the upgrade this way (instead of trying to get symlinks to work).

In addition to this, we've excluded 3 packages from being hoisted to the root of the project. This is purely to ensure the frontend has react, react-dom and formik available in it's relative node_modules folder so that it can build properly.

Speed improvements

PNPM is supposedly 2 - 3x times faster than the alternatives (Yarn, NPM, Lerna, etc.). As can be seen in the screenshot below of the build pipeline (left = PNPM + PNPM workspaces, right = current implementation of NPM + Lerna) we're now saving lots of time spent in CI

image (1)

Step NPM + Lerna PNPM + PNPM workspaces difference
Admin 11m 40s 8m 17s -3min 33s ✅
Frontend 22m 42s 15m 46s -6min 54s ✅

Overall build time difference:
-6min 55seconds ✅

NPM build + PNPM build side by side for future reference (PNPM left, NPM right):
image (2)

Relevant changes:

Updated the build pipeline to Install and use PNPM

  • Since Azure seems to not have a PNPM task, I've resorted to just using bash to run relevant PNPM commands.
  • Moved commonly used version specs (for things like Dotnet, Node etc.) to variables for better organization
  • Added appropriate spacing so it's easier to read
  • Removed old/unused parts of the build pipeline

Updated relevant READMEs

  • Updated public frontend README and removed old information
  • Updated main README due to changes in commands

General cleanup

  • Removed check-node-version package as PNPM natively protects against wrong node/PNPM versions being used with the engines field in package.json and engine-strict in .npmrc
  • Fixed type errors in FilterGroupDetails.tsx and IndicatorDetails.tsx that seem to have cropped up
  • Fixes flaky (at least for myself locally) Modal.test.tsx unit test
  • Fixes eslint warning in MapBlockInternal.tsx
  • Upgrades postcss-preset-env to v7+ to avoid a spammy warning in the admin webpack logs:
  • Removes the explicit nanoid and sanitize-html dependencies from next.config.js because IE11 isn't supported anymore and due to the following next-transpile-modules deprecation/error:
next-transpile-modules - DEPRECATED - fallbacking to previous module resolution system for module "explore-education-statistics-common/node_modules/nanoid", you can now just pass the name of the package to transpile and it will detect its real path without you having to pass a sub-module.

Upgraded admin babel packages

  • Upgraded admin babel packages to stop a problem occurring in CK editor whereby the editor simply wouldn't work and would crash whenever you navigated to parts of the admin app that rendered an editor. As part of this, amends needed to be made to the Admin's babel config. This was due to log spam in unit tests and when the admin initially started up. See https://stackoverflow.com/questions/67770240/webpacker-babel-spam-removal-message-not-clear-enough for an example of the warnings produced

image (4)

@luke-h1 luke-h1 force-pushed the EES-3976-3 branch 5 times, most recently from a7bc5c3 to e94a8ea Compare January 19, 2023 14:59
@luke-h1 luke-h1 marked this pull request as ready for review January 19, 2023 15:04
@luke-h1 luke-h1 force-pushed the EES-3976-3 branch 17 times, most recently from 1b333b4 to cbde2eb Compare January 19, 2023 21:32
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
azure-pipelines.dfe.yml Outdated Show resolved Hide resolved
azure-pipelines.dfe.yml Outdated Show resolved Hide resolved
azure-pipelines.dfe.yml Outdated Show resolved Hide resolved
src/explore-education-statistics-admin/package.json Outdated Show resolved Hide resolved
src/explore-education-statistics-frontend/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ntsim
Copy link
Collaborator

ntsim commented Jan 23, 2023

Those performance improvements are pure 🔥

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/explore-education-statistics-admin/package.json Outdated Show resolved Hide resolved
src/explore-education-statistics-frontend/package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@ntsim
Copy link
Collaborator

ntsim commented Jan 25, 2023

There are a bunch of unresolved comments ☝️

@luke-h1 luke-h1 force-pushed the EES-3976-3 branch 3 times, most recently from 621f2eb to 68843b6 Compare January 25, 2023 13:26
@luke-h1 luke-h1 merged commit 71de022 into dev Jan 26, 2023
@luke-h1 luke-h1 deleted the EES-3976-3 branch January 26, 2023 07:26
@luke-h1 luke-h1 restored the EES-3976-3 branch January 31, 2023 17:13
ntsim added a commit that referenced this pull request Jan 31, 2023
…6-3"

This reverts commit 71de022, reversing
changes made to d388209.
luke-h1 added a commit that referenced this pull request Jan 31, 2023
Revert "Merge pull request #3782 from dfe-analytical-services/EES-397…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants