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
Add typing to proto converters and fix breaks #4544
Conversation
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.
Couple Q's
"minBackoffSeconds", | ||
(seconds) => proto.durationFromSeconds(resolveInt(seconds)) | ||
); | ||
proto.renameIfPresent( |
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.
Isn't this identical to L374?
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.
yup
src/gcp/cloudfunctionsv2.ts
Outdated
return bytes / (1 << 20); | ||
bytes = bytes / (1 << 20); | ||
const typed = bytes as backend.MemoryOptions; | ||
if (backend.AllMemoryOptions.includes(typed)) { |
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.
should there be a !
here? like if typed
is not in AllMemoryOptions
then we'd throw a warning
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.
yup
After fixing #4543 I decided that I now know how to make
proto.renameIfPresent
more strongly typed and made a change that would have caught the error before it was ever a bug.Did that and started fixing the build breaks it caught. Since I noticed a lot of converter code for
build
was using the same key name, I also addedproto.convertIfPresent
.The build code was broken for scheduled functions. That's because the old
triggerParser
uses durations whereas the container contract uses seconds (so we can use number params). The code looks extra wonky becausebackend.Backend
also uses durations. As an exercise for the reader, someone should probably makebackend.Backend
use seconds and convert to/from duration inendpointFromFunction
andfunctionFromEndpoint
. duration is silly and should be in as little of our type system as possible.