Add support for useDate=false #211
Conversation
src/main.ts
Outdated
| @@ -576,7 +588,9 @@ function generateDecode(ctx: Context, fullName: string, messageDesc: DescriptorP | |||
| readSnippet = code`${type}.decode(reader, reader.uint32()).value`; | |||
| } else if (isTimestamp(field)) { | |||
| const type = basicTypeName(ctx, field, { keepValueType: true }); | |||
| readSnippet = code`${utils.fromTimestamp}(${type}.decode(reader, reader.uint32()))`; | |||
| readSnippet = options.useDate | |||
There was a problem hiding this comment.
Hm, I think it might be a simpler change to leave this line as it was, and then change line 589's "if isTimestamp(field)" to include "&& useDate".
That way we'll only get into this else if if its timestamp & use date is true, otherwise we'll go into the isMessage(field) block next, which I think should handle Timestamp's just fine, i.e. it will basically do that ${type}.decode that currently you have re-typed-out as a the : case.
...could probably do that for most of these if isTimestamp(field) changes?
There was a problem hiding this comment.
I wonder if isTimestamp should be renamed to isTimestampAsDate, and have it take both options & field, because (...in theory...?) everywhere that isTimestamp is called today, we really mean "...so that we can do special Date handling".
I guess the JSON serialization is the exception to that rule, where we want to detect Timestamp and do something special even if its not a Date.
There was a problem hiding this comment.
OK, I've simplified the code where I can and updated the codegen. Since we need isTimestamp to be separable from options.useDate for the JSON serialisation, I guess we could add a wrapper function that checks both, but I don't think that adds much value.
| return code`${from} !== undefined ? ${from}.toISOString() : null`; | ||
| return options.useDate | ||
| ? code`${from} !== undefined ? ${from}.toISOString() : null` | ||
| : code`${from} !== undefined ? ${utils.fromTimestamp}(${from}).toISOString() : null`; |
There was a problem hiding this comment.
Ah cool, keeping ISO strings on the wire...
| import "google/protobuf/timestamp.proto"; | ||
|
|
||
| message Metadata { | ||
| google.protobuf.Timestamp last_edited = 1; |
There was a problem hiding this comment.
This is a great small repro, thanks!
|
Thanks for the review, it will probably be Monday or Tuesday til we get to the fixes, but happy to make them |
|
Thanks for the updates Will. Is this blocked on anything @stephenh or just time to review it? |
Closes #208
use-date-falsethat demonstrates this caseNote for devs:
*.protofile you needcd integrations && ./update-bins.shprotoc --versionto be3.14.0for this to run properlycd integrations && ./codegen.sh../node_modules/.bin/ts-node ./codegen.ts ./use-date-false ./use-date-false/metadata.bin useDate=falseis faster to generate just one file, and works on macos