-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 incorrectly versioned bn.js type export #4418
fix incorrectly versioned bn.js type export #4418
Conversation
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Pull Request Test Coverage Report for Build 1296720633
💛 - Coveralls |
Hey there! Thank you for opening this PR, it might actually be a solution to the failing The I'm not sure why this error would pop up now, but it sounds similar to the problem you were describing by not including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files
dist/web3.min.js
dist/web3.min.js.map
shouldn't haven been updated, this only happens in release PRs
As mentioned in today's call, we should try to merge this for 1.6.1 @jdevcs to fix the failing tests. |
Sorry, I can make the changes requested now. Sorry for the delay! |
@spacesailor24 Thanks! This should be fixed now. Let me know if there's anything else that needs to be done before merging. |
@@ -472,3 +472,4 @@ Released with 1.0.0-beta.37 code base. | |||
- Format `block.baseFeePerGas` to number (#4330) | |||
- Correct `web3-eth-personal.sendTransaction` example in documentation (#4409) | |||
- Updated README to include Webpack 5 angular support instructions (#4174) | |||
- - Added @types/bn.js dependency to additional packages (notably web3-utils) so the type export aligns with the js. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- - Added @types/bn.js dependency to additional packages (notably web3-utils) so the type export aligns with the js. | |
- Added @types/bn.js dependency to additional packages (notably web3-utils) so the type export aligns with the js (#4418) |
Extra -
and missing PR number
Sounds good!
I totally could be wrong, but it was my impression that the issue was that it needed to be a regular dependency. If you are depending on this package and using typescript, you need the Does that make sense? |
Merging with failing `e2e_gnosis_dex` test as this may fix it * fix incorrectly versioned bn.js type export (#4418) * fix incorrectly versioned bn.js type export Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * WIP Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * Update CHANGELOG Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
Description
The
@types/bn.js
package is not listed as a dependency in all packages that have anindex.d.ts
file that callsrequire("bn.js")
. This causes downstream typescript users to resolve a type from a nested web3 dependency (ethereumjs-utils
), which usesbn.js@5.X.X
rather thanbn.js@4.X.X
as web3 does. The result is that web3 usesbn.js@4.X.X
in js, but exports thebn.js@5.X.X
type.For users, this can result in compilation errors if using the
bn.js
library directly and using the objects to interact with web3. In the worst case, this can cause typescript users to make invalid calls into these BN objects due to the type mismatch.Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
and testeddist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.