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

Bump @types/bn.js dependency to 5.1.1 #5640

Closed
wants to merge 5 commits into from
Closed

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Nov 18, 2022

Description

This PR sets a higher minimum version for the @types/bn.js dependency. In version 5.1.1, a new divmod function was added which is a breaking change on a patch version number release: a type defined from an import of @types/bn.js@v5.1.1 is not compatible with the BN type if defined from an import of @types/bn.js@v5.1.0.

Because the current loose specification allows either version to be used, sometimes NPM picks different versions for different web3 package or even for the same package directly vs. when included in another web3 package. This incompatibility causes code that previously worked fine due to type consistency to now fail compilation. Pinning all to .0 exactly would be less likely to cause breaking changes elsewhere or in the future, as maintainers of the typing package seem to not recognize this breaking change as such and could do something similar in the future. However, pinning to .0 would likely cause greater incompatibilities unless all projects which use web3 and have type interaction with the BN type also pin to .0 specifically, and the ^ notation is more common. As long as everything is consistent, it works, and failing forward makes a little more sense.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist for 1.x:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • [n/a] I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code. [n/a and ERROR: The filename, directory name, or volume label syntax is incorrect.]
  • I ran npm run build with success.
  • I have tested the built dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

@coveralls
Copy link

coveralls commented Nov 18, 2022

Pull Request Test Coverage Report for Build 3500766402

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.3%) to 72.233%

Totals Coverage Status
Change from base Build 3490104649: -2.3%
Covered Lines: 3414
Relevant Lines: 4445

💛 - Coveralls

@wbt
Copy link
Contributor Author

wbt commented Nov 19, 2022

The error is a timeout in connecting to Infura, and I don't (on this project) have privileges to force a re-run of a job with the same code, nor do I have a high opinion of adding extraneous commits to PRs that make it less focused.

@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Jan 19, 2023
@wbt
Copy link
Contributor Author

wbt commented Jan 19, 2023

Again, it's only stale because I don't have permissions on this project to retrigger a job that failed due to an Infura timeout.

@github-actions github-actions bot removed the Stale Has not received enough activity label Jan 20, 2023
@jdevcs
Copy link
Contributor

jdevcs commented Mar 6, 2023

merged #5885 into 1.x for these changes

@jdevcs jdevcs closed this Mar 6, 2023
@jdevcs jdevcs mentioned this pull request Mar 7, 2023
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.

None yet

3 participants