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.
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.
Good catch - I replaced a bit too much. Reverted everything that should not have been affected.
There was a problem hiding this comment.
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.
Given we're calling fromValue now we can/should remove the as string?
There was a problem hiding this comment.
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.
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.
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))
stephenh
force-pushed
the
feature/change_Long-fromString_to_Long-fromValue
branch
from
511c941
to
8eddf7b
Compare
stephenh
changed the title
…tness regarding already parsed objects
stephenh
force-pushed
the
feature/change_Long-fromString_to_Long-fromValue
branch
from
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