-
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
Avoid using **
with BigInt
#6506
Avoid using **
with BigInt
#6506
Conversation
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Deploying with
|
Latest commit: |
628cacf
|
Status: | ✅ Deploy successful! |
Preview URL: | https://5462bbbd.web3-js-docs.pages.dev |
Branch Preview URL: | https://6187-typeerror-cannot-conver.web3-js-docs.pages.dev |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6506 +/- ##
=======================================
Coverage 89.65% 89.66%
=======================================
Files 213 214 +1
Lines 8200 8206 +6
Branches 2220 2220
=======================================
+ Hits 7352 7358 +6
Misses 848 848
Flags with carried forward coverage won't be shown. Click here to find out more.
|
export const numberLimits: Map<string, { min: bigint; max: bigint }> = new Map([ | ||
[ | ||
'uint8', | ||
{ | ||
min: BigInt('0'), | ||
max: BigInt('255'), | ||
}, | ||
], | ||
[ | ||
'int8', | ||
{ | ||
min: BigInt('-128'), | ||
max: BigInt('127'), |
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.
Instead of adding this large file , why not to use function that you added bigintPower
in existing Map code .
const numberLimits = new Map<string, { min: bigint; max: bigint }>();
// precalculate all the limits
for (let i = 8; i <= 256; i += 8) {
numberLimits.set(`uint${i}`, {
min: BigInt(0),
max: BigInt(2) ** BigInt(i) - BigInt(1),
});
numberLimits.set(`int${i}`, {
min: -(BigInt(2) ** BigInt(i - 1)),
max: BigInt(2) ** BigInt(i - 1) - BigInt(1),
});
}
and see to save some lib size: as in this PR its
Total | 585 KB | 592 KB | 6.96 KB | 1.19%
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.
I though about reducing the execution time. But, sorry, I did not notice the size. I will modify to use the added bigintPower
.
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.
I changed it to use a loop with multiplication instead of power.
for (let i = 8; i <= 256; i += 8) { | ||
numberLimits.set(`uint${i}`, { | ||
min: BigInt(0), | ||
max: BigInt(2) ** BigInt(i) - BigInt(1), |
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.
Exponentiation Operator usage issue was solved by specifying min browser versions in one of issue?
Its supported in ECMAScript 2016 and
Chrome 52 | Edge 14 | Firefox 52 | Safari 10.1 | Opera 39
Jul 2016 | Aug 2016 | Mar 2017 | Mar 2017 | Aug 2016
so react with older browsers is causing this problem only, and if some one specify min browsers list in react config and use on ECMAScript 2016 supporting browser there is no issue?
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.
Unfortunately, the issue is present with the latest as well as any older version of React . And any other framework that uses babel (the latest version of babel and any older version as well).
And this is what happens:
- When building React app (to generate the build folder to be used for deployment), babel is used to tans-compile the code.
- Babel will convert every
**
toMath.pow
. - When browsing the page, an exception will be thrown because
Math.pow
does not work withBigInt
.
Currently, the only way to avoid this behavior is to add configuration to package.json
as mentioned in: babel/babel#13109 (comment) and applied in #6187 (comment)
And this MR propose changes so the users do not need to add configurations to their package.json
.
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.
I think it is better to have our own bigint pow function right now. and wait while Babel starts supporting it
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.
Thanks for this fix
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.
ok, Good, Thanks
Description
Fixes #6187
Type of change
Checklist:
npm run lint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:coverage
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.