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.

Sign up for GitHub

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

Jump to bottom

JSBI BigInt in read.js #74 #78

Merged
merged 3 commits into from
Sep 25, 2020
Merged

JSBI BigInt in read.js #74 #78

merged 3 commits into from
Sep 25, 2020

Conversation

bencmbrook
Copy link
Member

@bencmbrook bencmbrook commented Sep 24, 2020 •
edited
Loading

Original PR #74 from @vincaslt

@bencmbrook bencmbrook changed the title Proton mail jsbi read JSBI BigInt in read.js #74 Sep 25, 2020
@bencmbrook bencmbrook mentioned this pull request Sep 25, 2020
Closed
michaelfarrell76
michaelfarrell76 reviewed Sep 25, 2020
View reviewed changes
Copy link
Member

@michaelfarrell76 michaelfarrell76 left a comment

Choose a reason for hiding this comment

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

looks good, dont really know whats going on tho, maybe get @eligrey to take a look

eligrey
eligrey requested changes Sep 25, 2020
View reviewed changes
Copy link
Member

@eligrey eligrey left a comment

Choose a reason for hiding this comment

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

We should be using real BigInts when supported natively and fallback to JSBI as needed. Only ever using JSBI may decrease performance noticeably.

@bencmbrook
Copy link
Member Author

bencmbrook commented Sep 25, 2020 •
edited
Loading

I suggest keeping JSBI and polyfilling to bigint when it’s shipped widely. Interesting discussion here https://github.com/GoogleChromeLabs/jsbi

@eligrey
Copy link
Member

eligrey commented Sep 25, 2020

It seems like we can use https://github.com/GoogleChromeLabs/babel-plugin-transform-jsbi-to-bigint to auto-transform JSBI to BigInt.

I'm not sure if there is any reason preventing us from using the polyfill dynamically so that BigInt is used when supported.

@bencmbrook bencmbrook changed the base branch from master to bencmbrook/jsbi September 25, 2020 02:03
@bencmbrook
Copy link
Member Author

Okay, switching this to merge to a bigger JSBI feature branch

@bencmbrook bencmbrook merged commit 607ba9e into bencmbrook/jsbi Sep 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the ProtonMail-jsbi-read branch September 25, 2020 02:03
@ghost ghost assigned bencmbrook Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfarrell76 michaelfarrell76 michaelfarrell76 left review comments

@eligrey eligrey eligrey requested changes

Assignees

@bencmbrook bencmbrook

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bencmbrook @eligrey @michaelfarrell76 @vincaslt