RN-569 Upgrade yarn to v3 #3994

IgorNadj · 2022-06-24T00:38:28Z

Issue #RN-569

Changes:

Upgrade yarn to v3
Declare devDependencies of each package explicitly (see comment below)
Create common scripts for building, testing etc. that can be reused (see comment below)
Use babel instead of rollup
- admin-panel and ui-components
- This is simpler: can use common script, and gives the same result, see slack thread
Move e2e tests to their own package
- There is no dependency on web-frontend (after a tiny decoupling), and this way each package can have their dependencies easier to understand and maintain
Fix code
- ui-components: uniquely name exported members
- lesmis, web-frontend: use normal import from package, not @tupaia/package/some/file
- lesmis: move tests to standard dir so normal test script can be used

Outstanding issues

yarn run is slow due to issue with yarn [Feature] Improve performance of yarn run yarnpkg/berry#2575
- This will be worked around by introducing parallel builds in RN-346. Building in parallel will make the fact that there is an overhead for each build insignificant.
- The other thing we do is use npm run x instead of yarn x for builds, this saves around 4 seconds per invocation, and because we use chaining quite a bit this saves a lot of time.

Performance tests

Thing	BEFORE	AFTER	CHANGE%	What's changed
SKIP_BUILD_INTERNAL_DEPENDENCIES=1 yarn install (node_modules exists)	1.5s	3s	◽️about the same	Yarn v1 -> Yarn v3
SKIP_BUILD_INTERNAL_DEPENDENCIES=1 yarn install (no node_modules)	104s	57s	🟩 45% faster	Yarn v1 -> Yarn v3
ui-components build	16s	5s	🟩 68% faster	rollup -> babel, remove code splitting
admin-panel build	34s	34s	◽️ about the same	rollup -> babel
yarn build:internal-dependencies	325s	147s	🟩 54% faster	build-dev instead of build

IgorNadj · 2022-06-24T00:43:26Z

package.json

    "refresh-database": "yarn workspace @tupaia/database refresh-database --root ../../",
    "test-all": "node ./scripts/node/testAll --silent",
-    "validate": "./scripts/bash/validate.sh"
+    "validate": "./scripts/bash/validate.sh",
+    "package:build:ts": "cd $INIT_CWD && tsc -p tsconfig-build.json",


Scripts with colon can be called from any workspace. Combined with $INIT_CWD which is the workspace calling the script, we can reuse scripts in our workspaces. The other handy thing about this is that via this invocation, yarn loads root level npm packages, so each package doesn't need to have a copy of e.g. eslint declared as a devDependency.

See https://yarnpkg.com/getting-started/qa#how-to-share-scripts-between-workspaces

--

Edit: yarn run performance was quite slow, so I changed build scripts to use npm run --prefix ../../ instead. The others still use yarn x for consistency and compatibility. This is a bit inconsistent, but I will keep an eye on the issue and when it's resolved we will change everything back to yarn x again.

IgorNadj · 2022-06-24T00:46:10Z

packages/access-policy/package.json

    "test:coverage": "yarn test --coverage"
+  },
+  "devDependencies": {
+    "npm-run-all": "^4.1.5"


With Yarn2+, each package must explicitly declare its dependencies.
See https://yarnpkg.com/features/workspaces/#what-does-it-mean-to-be-a-workspace

IgorNadj · 2022-06-24T00:47:48Z

packages/central-server/package.json

-    "start-dev": "../../scripts/bash/backendStartDev.sh 9999",
-    "start-verbose": "LOG_LEVEL=debug npm run start-dev",
+    "start-dev": "yarn package:start:backend-start-dev 9999",
+    "start-verbose": "LOG_LEVEL=debug yarn start-dev",
    "test": "yarn workspace @tupaia/database check-test-database-exists && DB_NAME=tupaia_test mocha",


Packages which use non-standard scripts e.g. mocha declare them manually and have those dependencies explicitly specified. It's also a good push to move away from non-standard stuff.

IgorNadj · 2022-06-24T00:49:57Z

packages/devops/ci/tupaia-ci-cd.Dockerfile

@@ -43,6 +51,8 @@ RUN mkdir -p ./packages/data-lake-api
 COPY packages/data-lake-api/package.json ./packages/data-lake-api
 RUN mkdir -p ./packages/database
 COPY packages/database/package.json ./packages/database
+RUN mkdir -p ./packages/devops


I found that omitting some packages gives a different lockfile in Yarn2+, which means that the yarn install --frozen-lockfile below would fail, because it would want to change the lockfile. So I added them anyway. Adding these has no impact on CI/CD.

IgorNadj · 2022-06-24T00:51:19Z

packages/lesmis/src/api/queries/utils.js

@@ -3,7 +3,7 @@
 * Copyright (c) 2017 - 2020 Beyond Essential Systems Pty Ltd
 *
 */
-import { formatDateForApi, roundStartEndDates } from '@tupaia/ui-components/lib/chart';
+import { formatDateForApi, roundStartEndDates } from '@tupaia/ui-components';


With ui-components no longer using rollup, I changed these to normal imports.
Rollup is not needed because the tree shaking it was used for is handled by the frontends (react-scripts build), the final build size of our frontends is the same, e.g. web-frontend was 20MB, and it is still 20MB.

IgorNadj · 2022-06-24T00:54:02Z

packages/ui-components/.babelrc.js

@@ -26,5 +26,11 @@ module.exports = {
        loose: true,
      },
    ],
+    [
+      '@babel/plugin-proposal-private-property-in-object',


Fixes annoying console output spam:

[ui-components] Though the "loose" option was set to "false" in your @babel/preset-env config, it will not be used for @babel/plugin-proposal-private-property-in-object since the "loose" mode option was set to "true" for @babel/plugin-proposal-class-properties. [ui-components] The "loose" option must be the same for @babel/plugin-proposal-class-properties, @babel/plugin-proposal-private-methods and @babel/plugin-proposal-private-property-in-object (when they are enabled): you can silence this warning by explicitly adding [ui-components] ["@babel/plugin-proposal-private-property-in-object", { "loose": true }] [ui-components] to the "plugins" section of your Babel config.

IgorNadj · 2022-06-24T00:56:38Z

packages/ui-components/package.json

    "faker": "^4.1.0",
    "fast-glob": "^3.2.5",
-    "jsoneditor": "^9.0.0",
-    "moment": "^2.29.1",


We had a setup I don't understand here where some dependencies were marked as devDependencies even though they were actually used throughout src. Not sure how it worked before. I moved these to normal dependencies, and the final build size of our frontends is the same, e.g. web-frontend was 20MB, and it is still 20MB.

IgorNadj · 2022-06-24T03:51:26Z

packages/ui-components/src/components/Chart/ChartTooltip.js

@@ -149,7 +149,7 @@ const NoDataTooltip = ({ payload, periodGranularity }) => {
  );
 };

-export const Tooltip = props => {
+export const ChartTooltip = props => {


We had a couple different Tooltips and three different Tables in this package, all being exported at root level, causing errors when importing. I renamed them.

Following https://yarnpkg.com/getting-started/migration

This is a change from Yarn1 to Yarn2: "Only the dependencies depended upon by a workspace can be accessed". see https://yarnpkg.com/features/workspaces/#what-does-it-mean-to-be-a-workspace

The latest cypress/base docker image has node 14.19.0

This differentiates it from the normal build script, which is used for production releases. Typically we only want to build to a state where a developer can start work, or tests can be run on a package etc. One direct advantage is that admin panel no longer does a production build after each yyarn install.

See https://stackoverflow.com/a/51295237

biaoli0

@IgorNadj That's a chuck of works, nice one! Thanks for the e2e testing new package and some dependencies clean up, the chart makes me can't wait to use the Yarn new version.
Got a few small questions but it looks great overall.

biaoli0 · 2022-06-30T00:25:18Z

packages/web-frontend/package.json

-    "build:desktop": "SKIP_PREFLIGHT_CHECK=true BUILD_PATH='./build/desktop/' GENERATE_SOURCEMAP=false react-scripts build",
-    "build:mobile": "SKIP_PREFLIGHT_CHECK=true BUILD_PATH='./build/mobile/' GENERATE_SOURCEMAP=false REACT_APP_APP_TYPE=mobile react-scripts build",
+    "build:desktop": "BUILD_PATH='./build/desktop/' yarn package:build:react-scripts",
+    "build:mobile": "BUILD_PATH='./build/mobile/' yarn package:build:react-scripts",


Should we keep REACT_APP_APP_TYPE=mobile GENERATE_SOURCEMAP=false and SKIP_PREFLIGHT_CHECK=true ?

Good spotting on REACT_APP_APP_TYPE=mobile!
The other two env vars are set in package:build:react-scripts

biaoli0 · 2022-06-30T06:56:20Z

packages/admin-panel-server/package.json

@@ -16,6 +16,7 @@
  "private": true,
  "scripts": {
    "build": "yarn package:build:ts",
+    "build-dev": "yarn build-dev",


Should it be:
"build-dev": "yarn build"
Or we want to skip this package during internal build?

This looks like an old version of this PR, this is fixed in latest version.

IgorNadj · 2022-07-04T05:08:15Z

@billli0 I'm not sure if you reviewed an older version of this PR sorry, just pinged you to take a new look, thanks

biaoli0

Looks good!

tcaiger · 2022-07-05T01:24:37Z

Hey @IgorNadj I just wanted to mention for awareness that although tree shaking wasn't working well with rollup, bundling map and chart components separately was useful for saving bundle size in projects that do not use them such as PSSS and admin panel. I think those projects are not critical compared to Tupaia so I'm not worried about it. I think it could be a good idea to split out the chart components and map components into seperate packages in the mono-repo which would make this irrelevant anyway. There is an old ticket which includes this but it never got picked up. I'd be interested to hear if you agree and if so I might update that ticket.

IgorNadj marked this pull request as draft June 24, 2022 00:38

IgorNadj force-pushed the upgrade-yarn-v3 branch from 7ec1cb6 to 96207f0 Compare June 24, 2022 00:41

IgorNadj commented Jun 24, 2022

View reviewed changes

IgorNadj force-pushed the upgrade-yarn-v3 branch 2 times, most recently from 1923201 to 8772d37 Compare June 24, 2022 02:12

IgorNadj commented Jun 24, 2022

View reviewed changes

IgorNadj added 18 commits June 24, 2022 15:50

Update .gitignore

ade8f42

Upgrade to Yarn v3

89b1dda

Following https://yarnpkg.com/getting-started/migration

Bump local version of node to v14 to match prod

c6d3843

Initial yarn install after upgrade to yarn v3

5200baa

Add yarn workspaces plugin

5ebf29a

Lock node version, update CI to use yarn3

17d7ea6

Fix import

16d57bc

Add missing dependency

32546d3

Refactor: move __tests__ to src for consistency

f3b3f90

Refactor: uniquely name different table components

ad02a67

Refactor: uniquely name Tooltip components

0842be7

Localise dependencies to each workspace.

2bd2025

This is a change from Yarn1 to Yarn2: "Only the dependencies depended upon by a workspace can be accessed". see https://yarnpkg.com/features/workspaces/#what-does-it-mean-to-be-a-workspace

Reuse common scripts

c4d1560

Use babel instead of rollup for admin panel lib

cb9a4ba

Bump gold master to node v14

44568b3

Admin panel simpler lib build

8797869

Bump e2e Dockerfile to node 14

b636f00

The latest cypress/base docker image has node 14.19.0

Add missing dep

47c3e03

IgorNadj added 8 commits June 27, 2022 11:50

Merge branch 'dev' into upgrade-yarn-v3

9b3d326

Add missing dep for e2e testing

cf912d5

Refactor: extract e2e into own package

4c3c228

Simplify e2e Dockerfile

c1ec241

Add missing package

2d46548

Less verbose yarn output

bf3ac9d

Remove dependency on curl in e2e

9f55ad1

IgorNadj force-pushed the upgrade-yarn-v3 branch from 2f13a2a to 9f55ad1 Compare June 27, 2022 23:54

IgorNadj added 9 commits June 28, 2022 10:57

Add .npmrc files

edb3360

See https://stackoverflow.com/a/51295237

Use npm run instead of yarn for performance

c8c1e8e

Fix e2e test run

e2c8047

Fix broken build script

eecbfc5

Fix test e2e

f3c2c02

Fix test scripts

becc972

Fix test run scripts

bed436c

Update CI e2e env vars

f422847

Use yarn x for non-build scripts

292cef5

IgorNadj marked this pull request as ready for review June 28, 2022 05:49

IgorNadj changed the title ~~RN-346 Upgrade yarn to v3~~ RN-569 Upgrade yarn to v3 Jun 28, 2022

biaoli0 approved these changes Jul 1, 2022

View reviewed changes

IgorNadj added 2 commits July 4, 2022 15:00

PR fixups add missing env var

667cda8

Merge branch 'dev' into upgrade-yarn-v3

949a884

IgorNadj requested a review from biaoli0 July 4, 2022 05:07

biaoli0 approved these changes Jul 5, 2022

View reviewed changes

IgorNadj merged commit c91369e into dev Jul 6, 2022

IgorNadj deleted the upgrade-yarn-v3 branch July 6, 2022 23:43

IgorNadj mentioned this pull request Jul 14, 2022

Yarn v3 upgrade fixes #4029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN-569 Upgrade yarn to v3 #3994

RN-569 Upgrade yarn to v3 #3994

IgorNadj commented Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022

IgorNadj Jun 24, 2022

biaoli0 left a comment

biaoli0 Jun 30, 2022

IgorNadj Jul 4, 2022

biaoli0 Jun 30, 2022

IgorNadj Jul 4, 2022

IgorNadj commented Jul 4, 2022

biaoli0 left a comment

tcaiger commented Jul 5, 2022

RN-569 Upgrade yarn to v3 #3994

RN-569 Upgrade yarn to v3 #3994

Conversation

IgorNadj commented Jun 24, 2022 • edited

Issue #RN-569

Changes:

Outstanding issues

Performance tests

IgorNadj Jun 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj Jun 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj Jun 24, 2022 • edited

Choose a reason for hiding this comment

IgorNadj Jun 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biaoli0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj commented Jul 4, 2022

biaoli0 left a comment

Choose a reason for hiding this comment

tcaiger commented Jul 5, 2022

IgorNadj commented Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited

IgorNadj Jun 24, 2022 •

edited