feat: Add support for useDate=string #221
Conversation
| * 1970-01-01T00:00:00Z. Must be from 0001-01-01T00:00:00Z to | ||
| * 9999-12-31T23:59:59Z inclusive. | ||
| */ | ||
| seconds: number; |
There was a problem hiding this comment.
Is this value wrong? According to this, seconds can be int64 which can be string or Long (depending on other options). I think it's not related to this change though 🤔
There was a problem hiding this comment.
forceLong defaults to number so I think this is correct according to https://github.com/stephenh/ts-proto#number-types
|
Ah shoot, sorry, I forgot about this PR and already merged #211; can you rebase on top of master?
Fwiw just checking that for this to work, you can't use primitives w/o default values (like Are you able to handle/work around that? |
There was a problem hiding this comment.
@eqyiel Thanks so much for working on this. Looks good. I had a couple of suggestions for fixing the encode/decode methods and the fromJson method. Let me know if you have any questions about my suggestions.
eqyiel
force-pushed
the
feat/support-date-as-string
branch
from
9163a68
to
293e7c1
Compare
Co-authored-by: @actuosus <actuosus@users.noreply.github.com> Co-authored-by: @jessebutterfield <jessebutterfield@users.noreply.github.com>
eqyiel
force-pushed
the
feat/support-date-as-string
branch
from
293e7c1
to
c4977a2
Compare
There was a problem hiding this comment.
@stephenh @jessebutterfield thanks for reviewing, please take another look when you get time 😁
| * | ||
| * | ||
| * Example 6: Compute Timestamp from current time in Python. | ||
| * Example 5: Compute Timestamp from current time in Python. |
There was a problem hiding this comment.
I guess this file (and observable.bin and metadata.bin) changed because I used a different version of protoc than when they were compiled. I checked in these changes because after ./update-bins.sh && ./codegen.sh && npm run test -- -u there are no other unstaged files. It seems that this makes it consistent 😇
~/git/ts-proto/integration feat/support-date-as-string*
❯ protoc --version
libprotoc 3.13.0| function toTimestamp(dateStr: string): ${Timestamp} { | ||
| const date = new Date(dateStr); |
There was a problem hiding this comment.
I felt this was not smart because the variable named date can't be changed due to stuff outside of this function: https://github.com/eqyiel/ts-proto/blob/c4977a2a19ad565766d581c4e3d733d1667de150/src/main.ts#L371-L379
I don't think it's that much worse though, because that was already the case 😇
| const obj: any = {}; | ||
| message.id !== undefined && (obj.id = message.id); | ||
| message.timestamp !== undefined && | ||
| (obj.timestamp = message.timestamp !== undefined ? message.timestamp.toISOString() : null); |
There was a problem hiding this comment.
See my comment on the generateToJson code. I think this is unnecessarily checking for undefined twice.
There was a problem hiding this comment.
This is libprotoc 3.13.0, I guess it's the expected version otherwise there would be more changes than this.
stephenh
changed the title
# [1.79.0](v1.78.1...v1.79.0) (2021-04-02) ### Features * Add support for useDate=string ([#221](#221)) ([d967a9a](d967a9a))
|
🎉 This PR is included in version 1.79.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR builds on the best parts of #211 and #152 and makes the
useDateflag have three states, like suggested here.My use case is a bit unusual because I use this library to generate types only for use with a gRPC ↔ ️JSON gateway (no client implementation!) that follows the canonical JSON mapping, and what I really want is the option to have timestamps be strings in interfaces.
Because of that, I don't know if these encode and decode methods for
useDate=stringmake sense. I'd love to get some feedback from @actuosus and @jessebutterfield about this (because of their involvement in #152)!There are a few reasons:
useDate=string😁If #211 gets merged first, I'll update this. 🤞
Otherwise, I put appropriate
@co-authored-byattributions.