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

Implementing yarn workspaces 🚀 #900

Closed
wants to merge 25 commits into from
Closed

Conversation

evertonfraga
Copy link
Contributor

@evertonfraga evertonfraga commented Oct 5, 2020

This PR improves our monorepo package management system by a large margin. There's no need for bootstrap commands anymore.

Tasks:

@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #900 (ff12905) into ethereum-tests-submodule (d40c8df) will increase coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 76.35% <ø> (+1.31%) ⬆️
blockchain 77.39% <ø> (ø)
common 91.87% <ø> (ø)
ethash 82.08% <ø> (ø)
tx 100.00% <ø> (+13.95%) ⬆️
vm ?

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

@evertonfraga evertonfraga linked an issue Oct 6, 2020 that may be closed by this pull request
1 task
@evertonfraga
Copy link
Contributor Author

Current status:
Cache not fully restored, so ethereumjs-testing tests not being loaded correctly.

@evertonfraga evertonfraga mentioned this pull request Oct 28, 2020
1 task
@evertonfraga evertonfraga force-pushed the yarn-workspaces branch 2 times, most recently from 2d56353 to bf2fbb2 Compare October 29, 2020 15:49
@evertonfraga
Copy link
Contributor Author

Yarn has a problem loading git submodules from git dependencies. yarnpkg/yarn#1488

It is causing the failures shown here: https://github.com/ethereumjs/ethereumjs-vm/pull/900/checks?check_run_id=1327571708#step:7:191

@holgerd77
Copy link
Member

Could you summarize here a bit what problems this is going to solve respectively what properties might be expected from a switch? I've lost a bit track of this.

@evertonfraga
Copy link
Contributor Author

evertonfraga commented Nov 2, 2020

@holgerd77 sure!

The first reason is performance. There are issues with the combo npm + Lerna, which you seem to experience daily. Yarn workspaces is a native solution to manage dependencies in a multi-project setup, and it's just incredibly fast. A dirty lerna bootstrap takes >40s, while yarn under the same conditions takes only ~2s.

The other reason is to get rid of Lerna. It is highly opinionated and unexpectedly breaks things. I had poor experiences myself. It seems that @alcuadrado and @ricmoo already did the switch and that was my additional inspiration :)

@evertonfraga
Copy link
Contributor Author

@holgerd77 I forgot to mention that we'll also get rid of this weird behavior:

if you run npm install from the package directory, it will remove all links to the local packages, pulling all dependencies from npm. Run npm install from the root only.

@evertonfraga evertonfraga changed the base branch from master to ethereum-tests-submodule November 14, 2020 07:09
@evertonfraga evertonfraga marked this pull request as ready for review November 14, 2020 13:31
Base automatically changed from ethereum-tests-submodule to master November 16, 2020 09:12
@holgerd77
Copy link
Member

@evertonfraga Ok, I wouldn't dare to just merge this after the merge of #953, please give this an explicit note if you think it is ready for review! 😄

@holgerd77
Copy link
Member

(there is also still a WIP label on this PR)

@evertonfraga
Copy link
Contributor Author

@holgerd77 right!

I'd say to only merge this after you're all comfortable with the new submodule functionality, introduced at #953 .

@holgerd77
Copy link
Member

@evertonfraga I think everyone had the chance to give reaction on #953 and we can now review and merge here once updated (still open for comments though of course until merged).

@holgerd77
Copy link
Member

Short note: let's please wait on merging this in here until after the final releases on Tuesday. I assume there will be no side effects on this but with such a broad base tooling change you'll never know and I'd rather like to play safe here.

@holgerd77
Copy link
Member

This can theoretically be integrated now. 😄

@holgerd77
Copy link
Member

(@evertonfraga please let us know if you want to do yourself or if you want someone else to take this over)

@evertonfraga
Copy link
Contributor Author

@holgerd77 @ryanio hey :)

y'all want me to rebase this branch and put it up for review?

@holgerd77
Copy link
Member

@evertonfraga Hey ho, happy New Year 🎊, hope you had great holidays!

To your question: Yes. 😄

@Assawal
Copy link

Assawal commented Aug 8, 2021

This PR improves our monorepo package management system by a large margin. There's no need for bootstrap commands anymore.

Tasks:

@ryanio
Copy link
Contributor

ryanio commented Aug 11, 2021

Continuing this effort in #1395 with new npm v7 workspaces functionality 👍

@ryanio ryanio closed this Aug 11, 2021
@ryanio ryanio deleted the yarn-workspaces branch October 28, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize bootstrap duration
4 participants