Skip to content

ui: Use BigInt to represent 64 bit addresses#1932

Merged
javierhonduco merged 1 commit intoparca-dev:mainfrom
javierhonduco:use-bigint-for-addresses
Oct 19, 2022
Merged

ui: Use BigInt to represent 64 bit addresses#1932
javierhonduco merged 1 commit intoparca-dev:mainfrom
javierhonduco:use-bigint-for-addresses

Conversation

@javierhonduco
Copy link
Contributor

This is necessary as the maximum number that JS can represent is 2^53 [0].And show the addresses in the popover in hexadecimal.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

Test plan

image

Signed-off-by: Francisco Javier Honduvilla Coto javierhonduco@gmail.com

This is necessary as the maximum number that JS can represent is 2^53 [0].
And show the addresses in the popover in hexadecimal.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

- [0] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco requested a review from a team as a code owner October 19, 2022 16:28
@brancz
Copy link
Member

brancz commented Oct 19, 2022

cc @manojVivek @yomete I think there was an option in protobuf-ts to use BigInt by default to represent integers? I suppose we should do that?

Copy link
Contributor

@manojVivek manojVivek left a comment

Choose a reason for hiding this comment

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

Nice catch! 🔬

@manojVivek
Copy link
Contributor

cc @manojVivek @yomete I think there was an option in protobuf-ts to use BigInt by default to represent integers? I suppose we should do that?

Yeah looks like we can configure it and the default is BigInt already: https://github.com/timostamm/protobuf-ts/blob/master/MANUAL.md#bigint-support

@brancz
Copy link
Member

brancz commented Oct 19, 2022

Looks like we explicitly configure to not do that though

- long_type_string

@brancz
Copy link
Member

brancz commented Oct 19, 2022

Looks like I introduced that, if I recall correctly that was because that was how the previous protobuf library was doing it and it allowed me to do less work. We should probably change that.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

I do think we should switch the protobuf default, but better to merge this fix now and worry about the larger fix as a follow-up in the future. As far as I'm aware this should also be the only place where we care about this, so we might not need to do anything as a follow up, it just seems like the better thing to do long term.

@javierhonduco
Copy link
Contributor Author

Happy to either merge this PR as a quick fix, or close it if implementing the direct deserialisation of these values as BigInt isn't too hard 😄

@javierhonduco
Copy link
Contributor Author

We raced 😄 , let's merge this change and we can re-evaluate later on!

@javierhonduco javierhonduco enabled auto-merge (squash) October 19, 2022 16:39
@maxbrunet
Copy link
Member

maxbrunet commented Oct 19, 2022

Looks like I introduced that, if I recall correctly that was because that was how the previous protobuf library was doing it and it allowed me to do less work. We should probably change that.

Yup, I was reviewing that when I enabled TS strict mode, some browsers (Safari iirc) did not support it, but from the MDN link support looks good in recent versions

FYI I also mention it here: #1664 (comment)

It seems to me the cleanest way would to migrate from timostamm/protobuf-ts to bufbuild/protobuf-es. Maybe this can be extracted from the connect-web PR

https://github.com/bufbuild/protobuf-es/blob/main/docs/migrating.md

@javierhonduco javierhonduco merged commit 1dd552e into parca-dev:main Oct 19, 2022
@javierhonduco javierhonduco deleted the use-bigint-for-addresses branch October 24, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants