-
Notifications
You must be signed in to change notification settings - Fork 345
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
fix: use Long.fromValue instead of Long.fromString #405
fix: use Long.fromValue instead of Long.fromString #405
Conversation
Sorry for the failing build step - codegen somehow doesn't fail on my machine 🤷♂️ |
@@ -218,7 +218,7 @@ export const Int64Value = { | |||
|
|||
fromJSON(object: any): Int64Value { | |||
const message = { ...baseInt64Value } as Int64Value; | |||
message.value = object.value !== undefined && object.value !== null ? Long.fromString(object.value) : Long.ZERO; | |||
message.value = object.value !== undefined && object.value !== null ? Long.fromValue(object.value) : Long.ZERO; |
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.
Hm, I'm kind of surprised this line changed, b/c the only line you changed in main.ts
was:
code`Long.fromValue(${from} as string)`;
And there is no as string
here. Is there another change to main.ts
that is not in the PR?
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.
Good catch - I replaced a bit too much. Reverted everything that should not have been affected.
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.
After looking at it again, the corresponding place in main.ts
should be line 1013. It might be a good idea to also change that to fromValue
. But I wasn't sure if that's actually the right place. Maybe you could have a look, too.
integration/simple-long/simple.ts
Outdated
@@ -226,7 +226,7 @@ export const SimpleWithMap = { | |||
message.longLookup = {}; | |||
if (object.longLookup !== undefined && object.longLookup !== null) { | |||
Object.entries(object.longLookup).forEach(([key, value]) => { | |||
message.longLookup[key] = Long.fromString(value as string); | |||
message.longLookup[key] = Long.fromValue(value as string); |
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.
Given we're calling fromValue
now we can/should remove the as string
?
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 just tried it out and the type of value
is unknown
, so we need to cast to something. Long.fromValue
takes Long | number | string | {low: number, high: number, unsigned: boolean}
. I see the following options:
- leave the
as string
(because it actually should be string in JSON) - use
as Long | number | string | {low: number, high: number, unsigned: boolean}
(because that's what the method accepts) - use
as any
(we don't know what the JSON will contain, it's a hard cast anyway and can crash at runtime)
I think I'd still go with the as string
, but the decision is yours.
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.
Hm, maybe just as Long | string
since those are the cases we intend to support? I think if I saw just as string
at some point in the future, I'd forgot about this PR and think "well this should be just Long.fromString
. :-) But the as string | Long
I think would be a good reminder of our intent.
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.
Yes, good idea! I know the problem :-D I changed it and also added a link to this comment in the commit message.
…in fromJSON (see discussion here: stephenh#405 (comment))
511c941
to
8eddf7b
Compare
…tness regarding already parsed objects
8eddf7b
to
cd59030
Compare
Rebased on |
🎉 This PR is included in version 1.91.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks @stephenh! |
for improved robustness regarding already parsed objects
as discussed here