refactor!: use a union type for SignatureType #331
Conversation
| @@ -16,8 +16,22 @@ | |||
| // executing the client function. | |||
| export const FUNCTION_STATUS_HEADER_FIELD = 'X-Google-Status'; | |||
|
|
|||
| export enum SignatureType { | |||
There was a problem hiding this comment.
I don't agree with using string literals and arrays instead of enums. Enums provide benefits over literal values such as better typing, autocompletion, and less code here. We don't plan an arbitrary signature type until the FF contract changes.
This is also a breaking change to the API, not an internal refactor.
Could we discuss a bit more about the tradeoff in the PR / an issue?
There was a problem hiding this comment.
We're fine changing to the union types, but we shouldn't completely remove the enum as it's an exported interface, unless we have strong reason to introduce a breaking change.
For example, we had a customer complaint about a similar small interface change:
#216
There was a problem hiding this comment.
I don't think we give customer an opportunity to pass the value to use in any meaningful way (the signature type can only be provided as an env var or cli flag), so I don't think we should be too concerned about changing this. I see your point about why this is breaking change now. I will updated the conventional commit tag.
| @@ -108,7 +101,7 @@ getUserFunction(CODE_LOCATION, TARGET).then(userFunction => { | |||
|
|
|||
| SERVER.listen(PORT, () => { | |||
| ERROR_HANDLER.register(); | |||
| if (process.env.NODE_ENV !== NodeEnv.PRODUCTION) { | |||
| if (process.env.NODE_ENV !== 'production') { | |||
There was a problem hiding this comment.
I'm not seeing TS error for misconfigured strings with all.
For example, if I type:
if (process.env.NODE_ENV !== 'prodduction') {here, I won't get a TS error. I'd rather just be able to type NodeEnv. and see an autocompleted result of options and errors if I mistype.
This commit refactors the SignatureType enum into an array of strings declared [as const](microsoft/TypeScript#29510). This allows the SignatureType type to be expressed as a union type, which works a bit better when parsing a users provided string. This is some simple refactoring in preparation for declarative function signatures. BREAKING CHANGE: exported SignatureType type is converted from an enum to a union type
matthewrobertson
force-pushed
the
signature-union-type
branch
from
941f548
to
9109288
Compare
matthewrobertson
changed the title
This commit refactors the SignatureType enum into an array of strings declared as const. This allows the
SignatureTypetype to be expressed as a union type, which works a bit better when parsing a user provided string.This is some simple refactoring in preparation for declarative function signatures.
BREAKING CHANGE: exported SignatureType type is converted from an enum to a union type