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

Eliminate circular dependencies in web3 js #24729

Merged
merged 2 commits into from
Apr 27, 2022
Merged

Eliminate circular dependencies in web3 js #24729

merged 2 commits into from
Apr 27, 2022

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Apr 27, 2022

Problem

Circular dependencies are actually really really bad. Let's eliminate them and keep them out.

Musing: I haven't checked yet, but I wonder if this might have something to do with why some folks are having so much trouble doing tree-shaking.

Summary of Changes

  • Turn circular dependencies into a build error.
  • Actually fix the existing circular deps.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #24729 (b4554ba) into master (69725df) will increase coverage by 0.0%.
The diff coverage is n/a.

❗ Current head b4554ba differs from pull request most recent head a47de9c. Consider uploading reports for the commit a47de9c to get more accurate results

@@           Coverage Diff           @@
##           master   #24729   +/-   ##
=======================================
  Coverage    70.0%    70.0%           
=======================================
  Files          37       38    +1     
  Lines        2301     2303    +2     
  Branches      325      325           
=======================================
+ Hits         1612     1614    +2     
  Misses        573      573           
  Partials      116      116           

jstarry
jstarry previously approved these changes Apr 27, 2022
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

lgtm

*/
export const PACKET_DATA_SIZE = 1280 - 40 - 8;

export const SIGNATURE_LENGTH = 64;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add docs that state this length is measured in bytes (as opposed to base58 string length)

@mergify mergify bot dismissed jstarry’s stale review April 27, 2022 18:40

Pull request has been modified.

@steveluscher steveluscher merged commit 442e6c3 into solana-labs:master Apr 27, 2022
@steveluscher steveluscher deleted the eliminate-circular-dependencies-in-web3-js branch April 27, 2022 18:41
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
* chore: enable circular dependency warnings on build
* fix: eliminate circular dependencies in web3.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants