protobufjs
Use BigInt over Number for int64
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.
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 :)
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 - 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.
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?
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?
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.
@alexander-fenster - wondering if there is an ETA for v7 so I can start to work out how to take this forward?
@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.
Well, v7 has been released (woo!), and so hopefully we can start to pick this up @dcodeIO ?
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).
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.
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?
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.
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.
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.
I created a PR that's smaller in scope: #1908 which adds support for toObject({ longs: BigInt }).
the concerns about breaking changes seem like they are complicating this PR, but could we get some 👀 on @jtbandes's PR https://github.com/protobufjs/protobuf.js/pull/1908 ? it seems like a simple first step that would at least unblock users like myself who want to opt in to using BigInt 😄
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.
