monorepo: replace lerna with npm workspaces #1395

ryanio · 2021-08-09T21:37:59Z

This PR updates the monorepo from using lerna to npm workspaces, simplifying dev ux by removing the need for bootstrapping.

Typescript project references are used for linking packages during build time.

Run scripts in specific packages at root with e.g. npm run docs:build --workspace=@ethereumjs/vm or in all packages with --workspaces.

codecov · 2021-08-09T22:08:20Z

Codecov Report

Merging #1395 (5f307bd) into master (587ee70) will increase coverage by 1.19%.
The diff coverage is n/a.

Flag	Coverage Δ
block	`86.60% <ø> (ø)`
blockchain	`83.43% <ø> (-0.07%)`	⬇️
client	`83.38% <ø> (-0.48%)`	⬇️
common	`94.17% <ø> (-0.23%)`	⬇️
devp2p	`83.24% <ø> (+0.07%)`	⬆️
ethash	`82.83% <ø> (ø)`
tx	`88.36% <ø> (ø)`
vm	`82.79% <ø> (+3.45%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

… dist.browser

holgerd77 · 2021-08-10T07:37:13Z

Great, that sounds really promising on first sight! 😄

Several questions here:

If I understand correctly npm is at least somewhat attached to a specific Node version (or is this not correct, always thought so?) So do I then always have to run some latest Node version here? And does this more broadly affect the ability to test with different Node versions?
Are users of the packages - apart from devs directly working on the monorepo - in any way enforced to use npm 7?
npm run docs:build --workspace=@ethereumjs/vm: so is it not possible any more to run npm run docs:build from the package folder? I would find this pretty inconvenient especially for things like testing.

holgerd77 · 2021-08-10T10:18:23Z

Let's please wait until the next EthereumJS call until we merge this in.

holgerd77 · 2021-08-10T10:18:57Z

(doesn't mean that this can't be reviewed yet of course)

acolytec3

Once I installed the correct version of NPM and completely blew away the repo and started from scratch, everything seems to work fine and I like not having lerna around anymore. :-)

A couple of general comments/questions before approving:

I think we need to somehow specify in MONOREPO.md that you need to be on NPM version 7 to make contributions to the project. npm i threw errors when I tried to run it without being on npm7 but there weren't any obvious error messages telling me what was wrong.
What do you think about adding back the global lint/test scripts in the monorepo package.json? I ran both and (admittedly test is still running) they seem to work so might be nice to have those global scripts available.
I noticed that the packages/client/dist/bin/cli.js file isn't being ignored after running npm run build --workspaces from the root. Is this a necessary file for CI purposes? I guess it's probably not a big deal but it could be a pain to deal with if we have that file sneaking into every new commit to the client package.

ryanio · 2021-08-10T18:06:51Z

Thanks guys for taking an initial look! I'm excited for these improvements.

If I understand correctly npm is at least somewhat attached to a specific Node version (or is this not correct, always thought so?) So do I then always have to run some latest Node version here? And does this more broadly affect the ability to test with different Node versions?

Yes usually a Node version has a specific npm version bundled with it (see this table).

It's easy to upgrade to use npm v7 though regardless of the node version you are on, simply npm i -g npm@7, which I am doing in the ci.

Are users of the packages - apart from devs directly working on the monorepo - in any way enforced to use npm 7?

I don't think so, only developers of the monorepo.

npm run docs:build --workspace=@ethereumjs/vm: so is it not possible any more to run npm run docs:build from the package folder? I would find this pretty inconvenient especially for things like testing.

Yes you still can run it in a package folder, this is just a convenience when on root level to specify which package(s) the cmd should run.

Once I installed the correct version of NPM and completely blew away the repo and started from scratch, everything seems to work fine and I like not having lerna around anymore. :-)

wohoo

I think we need to somehow specify in MONOREPO.md that you need to be on NPM version 7 to make contributions to the project. npm i threw errors when I tried to run it without being on npm7 but there weren't any obvious error messages telling me what was wrong.

Yes this is actually what I was trying to do with the engines -> npm field but it just outputs a small warning instead of halting. Good point though, I will add extra documentation and I'm also thinking of adding a preinstall script that will grep npm -v and exit with a friendly error suggesting to do npm i -g npm@7 if <v7 for workspaces support.

What do you think about adding back the global lint/test scripts in the monorepo package.json? I ran both and (admittedly test is still running) they seem to work so might be nice to have those global scripts available.

I can add them back, it's a bit redundant though, because you can just run the script you want e.g. npm run lint:fix --workspaces to run the cmd (I updated the documentation to reflect this). I thought it was a bit redundant to add it to the root package.json and also it screws up if you want to run e.g. npm run lint:fix --workspace=@ethereumjs/vm then it would call the root script to run in all packages.

I noticed that the packages/client/dist/bin/cli.js file isn't being ignored after running npm run build --workspaces from the root. Is this a necessary file for CI purposes? I guess it's probably not a big deal but it could be a pain to deal with if we have that file sneaking into every new commit to the client package.

I was having a bit of trouble with this one, without that file present npm install was trying to chmod the file since it is defined as a bin path in client/package.json - and it was erroring out on 404 since the file is not present before building - I'm going to fiddle around with this one this morning and see if I can get a better/cleaner solution that does not need to define that file as a placeholder since as you mention it doesn't work when the file gets updated with code then tries to check into git.

ryanio · 2021-08-10T21:39:50Z

ok, I wrote a nice little preinstall script to assist with npm version detection that outputs either:

npm >=v7 satisfied

or:

npm version 7 or greater is required for workspaces support. Please update with "npm i -g npm@7"

will push up a commit along with some other updates this afternoon.

…esolved chmod error with preinstall script workaround)

alcuadrado · 2021-08-11T18:04:19Z

.github/workflows/block-build.yml

@@ -34,17 +34,20 @@ jobs:
        uses: actions/cache@v2
        id: cache
        with:
-          key: Block-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('**/package-lock.json') }}
+          key: cache-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('**/package-lock.json') }}


I believe the latest version of actions/setup-node does this automatically for you. I haven't tried that yet, but looks promising.

https://github.com/actions/setup-node#caching-packages-dependencies

oh, nice, thanks! that does look a lot simpler. let me give it a try.

acolytec3 · 2021-08-11T19:22:17Z

LGTM!

holgerd77 · 2021-08-11T20:35:07Z

package.json

@@ -1,6 +1,12 @@
 {
  "name": "root",
  "private": true,
+  "workspaces": [
+    "packages/*"


There is the ethereumjs-testing submodule implicitly included here, might this have any side effects?

that's a great point, it does seem to be ignoring it so far as a workspace.

when running commands at root with --workspaces the command is not executed in ethereumjs-testing.

holgerd77 · 2021-08-11T20:37:38Z

Also gave this a brief review, looks really great! 😄 🎉 Will be brave and directly merge.

ryanio added PR state: needs review package: monorepo labels Aug 9, 2021

ryanio force-pushed the npm-workspaces branch from 23a07b6 to 1b09e2d Compare August 9, 2021 23:24

ryanio added PR state: WIP and removed PR state: needs review labels Aug 9, 2021

ryanio force-pushed the npm-workspaces branch 2 times, most recently from c84fca9 to d141ad9 Compare August 10, 2021 00:25

replace lerna with npm workspaces and typescript references

539338d

ryanio force-pushed the npm-workspaces branch 2 times, most recently from 3103ef2 to 289f878 Compare August 10, 2021 00:53

try building all packages so karma doesn't error on trying to resolve…

bc092b0

… dist.browser

ryanio force-pushed the npm-workspaces branch from 289f878 to bc092b0 Compare August 10, 2021 01:03

add missing npm v7 update for test-client-cli

41cae65

ryanio added PR state: needs review and removed PR state: WIP labels Aug 10, 2021

acolytec3 self-requested a review August 10, 2021 10:14

acolytec3 reviewed Aug 10, 2021

View reviewed changes

add preinstall script checkNpmVersion, remove cli placeholder file (r…

fc537e4

…esolved chmod error with preinstall script workaround)

ryanio force-pushed the npm-workspaces branch from 93e9329 to fc537e4 Compare August 11, 2021 03:38

ryanio mentioned this pull request Aug 11, 2021

Implementing yarn workspaces 🚀 #900

Closed

2 tasks

ryanio added 2 commits August 11, 2021 09:38

unify cache keys

7426c84

Merge branch 'master' into npm-workspaces

5f307bd

alcuadrado reviewed Aug 11, 2021

View reviewed changes

acolytec3 approved these changes Aug 11, 2021

View reviewed changes

holgerd77 reviewed Aug 11, 2021

View reviewed changes

holgerd77 merged commit 66d02e1 into master Aug 11, 2021

holgerd77 deleted the npm-workspaces branch August 11, 2021 20:38

ryanio mentioned this pull request Aug 11, 2021

ci: simplify dependency cache code #1401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monorepo: replace lerna with npm workspaces #1395

monorepo: replace lerna with npm workspaces #1395

ryanio commented Aug 9, 2021 •

edited

Loading

codecov bot commented Aug 9, 2021 •

edited

Loading

holgerd77 commented Aug 10, 2021

holgerd77 commented Aug 10, 2021

holgerd77 commented Aug 10, 2021

acolytec3 left a comment •

edited

Loading

ryanio commented Aug 10, 2021 •

edited

Loading

ryanio commented Aug 10, 2021 •

edited

Loading

alcuadrado Aug 11, 2021

ryanio Aug 11, 2021

acolytec3 commented Aug 11, 2021

holgerd77 Aug 11, 2021

ryanio Aug 11, 2021

holgerd77 commented Aug 11, 2021

monorepo: replace lerna with npm workspaces #1395

monorepo: replace lerna with npm workspaces #1395

Conversation

ryanio commented Aug 9, 2021 • edited Loading

codecov bot commented Aug 9, 2021 • edited Loading

Codecov Report

holgerd77 commented Aug 10, 2021

holgerd77 commented Aug 10, 2021

holgerd77 commented Aug 10, 2021

acolytec3 left a comment • edited Loading

Choose a reason for hiding this comment

ryanio commented Aug 10, 2021 • edited Loading

ryanio commented Aug 10, 2021 • edited Loading

alcuadrado Aug 11, 2021

Choose a reason for hiding this comment

ryanio Aug 11, 2021

Choose a reason for hiding this comment

acolytec3 commented Aug 11, 2021

holgerd77 Aug 11, 2021

Choose a reason for hiding this comment

ryanio Aug 11, 2021

Choose a reason for hiding this comment

holgerd77 commented Aug 11, 2021

ryanio commented Aug 9, 2021 •

edited

Loading

codecov bot commented Aug 9, 2021 •

edited

Loading

acolytec3 left a comment •

edited

Loading

ryanio commented Aug 10, 2021 •

edited

Loading

ryanio commented Aug 10, 2021 •

edited

Loading