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

fix: Equalizing dependency versions #782

Merged
merged 7 commits into from
Jun 22, 2020
Merged

fix: Equalizing dependency versions #782

merged 7 commits into from
Jun 22, 2020

Conversation

evertonfraga
Copy link
Contributor

@evertonfraga evertonfraga commented Jun 17, 2020

This PR equalizes package versions.

  • On the developer side, It will enable us to use Lerna's automatic dependency hoisting, that installs common dependencies only once.
  • On the CI side, it would make overall cache size smaller.
  • On the project quality and maintainability, adds consistency.

Remaining work that is being done (or will be done) in other PRs:

Related: #730, #770.

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #782 into master will decrease coverage by 5.96%.
The diff coverage is 88.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   90.47%   84.50%   -5.97%     
==========================================
  Files          49       16      -33     
  Lines        3170     1265    -1905     
  Branches      485      249     -236     
==========================================
- Hits         2868     1069    -1799     
+ Misses        204      126      -78     
+ Partials       98       70      -28     
Flag Coverage Δ
#account 94.59% <ø> (+0.47%) ⬆️
#block 80.02% <88.00%> (-2.33%) ⬇️
#blockchain 84.71% <100.00%> (-4.10%) ⬇️
#common 93.60% <ø> (-0.19%) ⬇️
#ethash 86.30% <ø> (ø)
#tx 94.02% <ø> (-0.14%) ⬇️
#vm ?
Impacted Files Coverage Δ
packages/block/src/header-from-rpc.ts 66.66% <ø> (ø)
packages/tx/src/fake.ts 90.00% <ø> (ø)
packages/tx/src/transaction.ts 94.73% <ø> (ø)
packages/block/src/block.ts 70.47% <66.66%> (-0.68%) ⬇️
packages/block/src/from-rpc.ts 86.66% <100.00%> (ø)
packages/block/src/header.ts 66.94% <100.00%> (ø)
packages/blockchain/src/callbackify.ts 65.21% <100.00%> (-9.79%) ⬇️
packages/blockchain/src/index.ts 83.54% <100.00%> (-4.25%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb82f40...cf67e2a. Read the comment docs.

@evertonfraga evertonfraga mentioned this pull request Jun 19, 2020
10 tasks
@evertonfraga evertonfraga marked this pull request as ready for review June 19, 2020 13:57
@evertonfraga evertonfraga requested review from ryanio and alcuadrado and removed request for ryanio June 22, 2020 12:51
@evertonfraga evertonfraga requested review from ryanio, holgerd77 and jochem-brouwer and removed request for alcuadrado June 22, 2020 12:51
@evertonfraga evertonfraga self-assigned this Jun 22, 2020
@jochem-brouwer
Copy link
Member

jochem-brouwer commented Jun 22, 2020

LGTM. Did you use any automated software to equalize these dependencies?

Is it an idea to centralize (i.e. in the root) the packages which multiple packages in the monorepo use?

EDIT: Hmm. Looks like the checks are not succesful.

jochem-brouwer
jochem-brouwer previously approved these changes Jun 22, 2020
@jochem-brouwer jochem-brouwer self-requested a review June 22, 2020 14:12
@jochem-brouwer jochem-brouwer dismissed their stale review June 22, 2020 14:12

Checks are failing

@evertonfraga
Copy link
Contributor Author

evertonfraga commented Jun 22, 2020

@jochem-brouwer thanks for the review.

Although moving dependencies to the root package.json would work in our machines and CI, that would not work whenever people installed individual libraries.

Patricio raised a PR (#770) with a script that helps detecting sub-packages with different versions, so it will help us detecting these early and often.

LGTM. Did you use any automated software to equalize these dependencies?

I normalized those manually, doing some regular expressions and find/replaces.

And I didn't want to update them all to latest major, because that requires more time investment, and my goal for now is just to simplify the dependency tree.

Regarding failing checks: linting problems 😣

@evertonfraga
Copy link
Contributor Author

Right, this is now ready for review again. I was using the same lint from another PR 😄

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

@evertonfraga evertonfraga merged commit f4a1266 into master Jun 22, 2020
@evertonfraga evertonfraga deleted the equalizing-deps branch June 22, 2020 21:01
"@ethereumjs/config-prettier": "^1.1.0",
"@ethereumjs/config-tsc": "^1.1.0",
"@ethereumjs/config-tslint": "^1.1.0",
"@ethereumjs/config-nyc": "^1.1.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding: this is just a cosmetic change, right? Or does this make a difference on CI caching or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holgerd77 Even though the version ranges result in the same version being used, it is a requirement for lerna bootsrap --hoist to work properly. See big chunk of warnings described in #730.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or more explicitly: the repo must use the same exact versions so they get properly hoisted.

jochem-brouwer pushed a commit that referenced this pull request Jun 23, 2020
* Normalizing dependency versions

* lint: Side-effects of package updates

* Updates merkle-patricia-tree dep

* test: side-effect of updating nyc

* Updates @types/tape dep

* refactor: using tape whithout paths

* Updates -tx and ethereumjs-util
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.

4 participants