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

Use BigInt over Number for int64 #1557

Closed
wants to merge 3 commits into from
Closed

Conversation

mjgp2
Copy link

@mjgp2 mjgp2 commented Mar 9, 2021

This PR removes the need for the Long package, and instead of Number uses BigInt as standard for all int64 types.

I realise that this would be a breaking change, but it seems like it would be the right design decision for the next major version of this library?

All unit tests pass, and needed to switch to terser from uglify.

Node versions before 10 would not be supported. However, Node 10 LTS is EOL on 2021-04-30.

This was referenced Mar 9, 2021
@alexander-fenster
Copy link
Contributor

Hi @mjgp2,

Thank you for this PR. We probably cannot include it in v7.0.0 (that is coming soon) but we'll think if we can drop long in the next semver major. At least this would be a major pain for us at Google to convert all the client libraries (which reexport types from protobuf.js), and I can only imagine how disruptive it can be for others. Also, if you see how the long support is implemented, there's a configuration option that enables or disables long. It maybe makes sense to think if we can make this less breaking by doing it in steps:

(1) start accepting BigInt where we currently accept long when serializing;
(2) make it configurable to be able to choose if we dump long or BigInt.

Also, dropping support for Node.js v4-6-8 is a big decision.

Of course, Cc: @dcodeIO on this.

I promise we'll get back to reviewing this PR when we're done with releasing v7.0.0 :)

@mjgp2
Copy link
Author

mjgp2 commented Mar 15, 2021

Thanks for your reply.

Yes, I think that definitely you could support Number, BigInt, String and Long for int64 types. My thinking here was firstly to understand the codebase and to swap Long for BigInt being simpler than adding the extra branching logic, and then hopefully get some feedback.

I would suggest that it is considered whether support for Number for int64 should be dropped - there are silent overflows today I think within the codebase that are really data corruption.

@ardatan
Copy link

ardatan commented Apr 20, 2021

Any progress on this PR?

@mjgp2
Copy link
Author

mjgp2 commented Jun 25, 2021

@ardatan - you can use the branch on my repo, it works, i.e. in your package.json

  "resolutions": {
    "**/protobufjs": "mjgp2/protobuf.js#bigint"
  },

I understand it's going to be targeted for version 8. I'm happy to help when the time comes - it's still my thinking that Number support for int64 should be dropped in version 8 because it will corrupt data.

@hornta
Copy link

hornta commented Sep 17, 2021

Sorry for pinging a bunch of people but me and my colleagues are really looking forward to this PR being merged and published so we are looking for a status on this. Is it stuck on something other than having conflicts? Can we help out in any way?

@mjgp2
Copy link
Author

mjgp2 commented Sep 17, 2021

I'm happy to take it forward, but I think I was waiting for v7 to be released first. Not sure where that is right now?

@mouafa
Copy link

mouafa commented Dec 1, 2021

Well done @mjgp2

Is there some sort of a timeline with a release date for v7 and v8?, I'm really looking forward to dropping the ~40KB long.js from our codebase.

image

@goya
Copy link

goya commented Dec 1, 2021

} else
object.uint64Val = options.longs === String ? "0" : 0;
var long = new $util.LongBits(NaN, NaN, undefined);
object.uint64Val = options.longs === String ? long.toBigInt().toString() : options.longs === BigInt ? long.toBigInt().toString() : long;

Choose a reason for hiding this comment

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

Potential bug fix to default value here if the user chooses to use { longs: BitInt }

Suggested change
object.uint64Val = options.longs === String ? long.toBigInt().toString() : options.longs === BigInt ? long.toBigInt().toString() : long;
object.uint64Val = options.longs === String ? long.toBigInt(true).toString() : options.longs === BigInt ? long.toBigInt(true): long;

@mjgp2
Copy link
Author

mjgp2 commented Jan 25, 2022

@alexander-fenster - wondering if there is an ETA for v7 so I can start to work out how to take this forward?

@alexander-fenster
Copy link
Contributor

@mjgp2 Sorry, I don't have access to releases in this repository so I cannot help here. Your best bet would be to ask the owner, @dcodeIO.

* Zero hash.
* @memberof util.LongBits
* @type {string}
* Constructs new long bits from the specified number.

Choose a reason for hiding this comment

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

Suggested change
* Constructs new long bits from the specified number.
* Constructs new long bits from the specified bigint.

* @memberof util.LongBits
* @type {string}
* Constructs new long bits from the specified number.
* @param {number} value Value

Choose a reason for hiding this comment

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

Suggested change
* @param {number} value Value
* @param {bigint} value Value

*/
var zeroHash = LongBits.zeroHash = "\0\0\0\0\0\0\0\0";
LongBits.fromBigInt = function fromNumber(value) {

Choose a reason for hiding this comment

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

Suggested change
LongBits.fromBigInt = function fromNumber(value) {
LongBits.fromBigInt = function fromBigInt(value) {

@mahnunchik
Copy link

Any news?

@mjgp2
Copy link
Author

mjgp2 commented Jul 19, 2022

Well, v7 has been released (woo!), and so hopefully we can start to pick this up @dcodeIO ?

@alexander-fenster
Copy link
Contributor

Yes, we finally got v7 out - and also dropped support for older versions of Node.js (we start from 12 now). I should say I'm not sure how we properly release this if we make this change, this will be a huge breaking change for many users. I wonder if we could somehow use the configuration option to choose which type to use (and have this option for pbts as well, since the generated types are also affected).

Comment on lines +59 to +60
var hi = Number(value >> 32n) | 0;
var lo = Number(value - ( BigInt(hi) << 32n ) ) | 0;

Choose a reason for hiding this comment

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

I've been unable to correctly encode uint64 fields with the max field value of 18446744073709551615n without this change:

Suggested change
var hi = Number(value >> 32n) | 0;
var lo = Number(value - ( BigInt(hi) << 32n ) ) | 0;
var hi = Number(value >> 32n);
var lo = Number(value - ( BigInt(hi) << 32n ) );

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it by juzibot#7

achingbrain added a commit to ipfs/protons that referenced this pull request Aug 10, 2022
Ports just the parts of protobufjs we use to typescript and integrates
the changes from protobufjs/protobuf.js#1557
to have native BigInt support.
@hallahan
Copy link

hallahan commented Sep 2, 2022

I'm assuming bigint will perform significantly better since it's a native type. If we aren't going to merge this, would it make sense to fork this repo and have an npm package under a different name?

Having an object wrapping two Numbers uses significantly more memory as well, so wouldn't it be advantageous to remove Long? We could add a Long shim around bigint for backwards compatibility.

@mahnunchik
Copy link

Any news?

1 similar comment
@Maligosus
Copy link

Any news?

@azizghuloum
Copy link

azizghuloum commented Dec 28, 2022

I'm not a maintainer so speaking for myself here.

Honestly I don't think this can be merged due to breaking compatibility.

What needs to be done is:

First: Add support for bigints as an option (so people can migrate to it).

Then: Maybe in 5 years, drop support of Long (since no one would be using it by then, hopefully).

It's a great patch and I wish it were merged already. I need bigints myself and don't want to start a new project with legacy junk from the start. However, I can imagine that there are millions of lines of code affected by this change and that's why it's been 2 years since the patch was written and it's still not merged. It breaks people's code.

Yes, the maintainers can merge it and force people to migrate or stay behind. However, people will generally not like that.

@dcodeIO @alexander-fenster @mjgp2 Is this kinda what the situation is? Would this be a reasonable approach to make progress?

@hallahan
Copy link

Why not just merge this and release a full version bump? That's what versions are for. Software that wants to keep using Long can stick with an older version.

Putting two Numbers (which are float64) in an Object to handle integers over 52 bits is a very inefficient solution.

@goya
Copy link

goya commented Dec 28, 2022

i agree merge this. i vote we merge this 1 year after bigint has been released in stable JS versions.

“millions of lines of code” = code generated by this library.

@mjgp2
Copy link
Author

mjgp2 commented Dec 31, 2022

i agree merge this. i vote we merge this 1 year after bigint has been released in stable JS versions.

Bigint is in all major browsers for over 2 years and since Node 10.

I think it's waiting for the owner(s) to decide how to proceed - there is a decision to be made about if it is necessary to support both Long and BigInt. I do not generate code with this library, just use it dynamically, so do not have much context on how it affects that.

@mahnunchik
Copy link

@alexander-fenster ping from 2023

@phoenix741
Copy link

I wait for native bigint too

("d%s=o.longs===String?n.toString():o.longs===Number?n.toNumber():n", prop)
("}else")
("d%s=o.longs===String?%j:%i", prop, field.typeDefault.toString(), field.typeDefault.toNumber());
("var n=new util.LongBits(%i,%i,%j)", field.typeDefault.low, field.typeDefault.high, field.typeDefault.unsigned)
Copy link

Choose a reason for hiding this comment

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

This code will define n for every long field, which cause failure on es6 build(-w es6).
You should check if n has declared in previous field.
Like this : seo-rii@1dac06f#diff-444e086c8038faf674140e30c0597966b9a9102859bf04422d247118adedd08c

@jtbandes
Copy link

jtbandes commented Jul 6, 2023

I created a PR that's smaller in scope: #1908 which adds support for toObject({ longs: BigInt }).

achingbrain added a commit to ipfs/protons that referenced this pull request Oct 13, 2023
Ports just the parts of protobufjs we use to typescript and integrates
the changes from protobufjs/protobuf.js#1557
to have native BigInt support.
@jmondo
Copy link

jmondo commented Dec 20, 2023

the concerns about breaking changes seem like they are complicating this PR, but could we get some 👀 on @jtbandes's PR #1908 ? it seems like a simple first step that would at least unblock users like myself who want to opt in to using BigInt 😄

@shanghaikid
Copy link

2024, any progress?

@mjgp2
Copy link
Author

mjgp2 commented Feb 21, 2024

I don't have bandwidth to look at this any further - hopefully someone will be able to get some form of support for BigInt in.

@mjgp2 mjgp2 closed this Feb 21, 2024
@hallahan
Copy link

😢

@mahnunchik
Copy link

😢 😢 😢

@cupen
Copy link

cupen commented Mar 21, 2024

Lol.
It seems that int64/uint64 is a big problem to javascript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.