-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
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
luke-h1
force-pushed
the
EES-3976-3
branch
5 times, most recently
from
January 19, 2023 14:59
a7bc5c3
to
e94a8ea
Compare
luke-h1
requested review from
amyb-hiveit,
ntsim,
a user,
benoutram,
duncan-at-hiveit and
N-moh
January 19, 2023 15:05
luke-h1
force-pushed
the
EES-3976-3
branch
17 times, most recently
from
January 19, 2023 21:32
1b333b4
to
cbde2eb
Compare
ntsim
requested changes
Jan 23, 2023
src/explore-education-statistics-common/src/modules/charts/components/LineChartBlock.tsx
Outdated
Show resolved
Hide resolved
Those performance improvements are pure 🔥 |
luke-h1
force-pushed
the
EES-3976-3
branch
3 times, most recently
from
January 24, 2023 16:11
35fecac
to
ed6448d
Compare
luke-h1
force-pushed
the
EES-3976-3
branch
5 times, most recently
from
January 24, 2023 19:24
d803c1c
to
359c152
Compare
ntsim
requested changes
Jan 25, 2023
...ore-education-statistics-admin/src/pages/release/footnotes/components/FilterGroupDetails.tsx
Outdated
Show resolved
Hide resolved
...plore-education-statistics-admin/src/pages/release/footnotes/components/IndicatorDetails.tsx
Outdated
Show resolved
Hide resolved
There are a bunch of unresolved comments ☝️ |
luke-h1
force-pushed
the
EES-3976-3
branch
3 times, most recently
from
January 25, 2023 13:26
621f2eb
to
68843b6
Compare
ntsim
approved these changes
Jan 25, 2023
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
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.
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 intopnpm i
.pnpm i
will install the root dependencies along with all theworkspace
packages defined inpnpm-workspace.yaml
When it comes to installing new dependencies, you simply modify the relevant
package.json
and runpnpm 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.
Install PNPM & project dependencies
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-symlinkednode_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
andformik
available in it's relativenode_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
Overall build time difference:
-6min 55seconds ✅
NPM build + PNPM build side by side for future reference (PNPM left, NPM right):
Relevant changes:
Updated the build pipeline to Install and use PNPM
PNPM
task, I've resorted to just using bash to run relevant PNPM commands.Updated relevant READMEs
General cleanup
FilterGroupDetails.tsx
andIndicatorDetails.tsx
that seem to have cropped upModal.test.tsx
unit testMapBlockInternal.tsx
postcss-preset-env
to v7+ to avoid a spammy warning in the admin webpack logs:nanoid
andsanitize-html
dependencies fromnext.config.js
because IE11 isn't supported anymore and due to the followingnext-transpile-modules
deprecation/error:Upgraded admin babel packages