Define type-specific getters for SSRC #2519
Conversation
| constructor( | ||
| private readonly source: ValueSource, | ||
| private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { } | ||
| asBoolean(): boolean { |
There was a problem hiding this comment.
should we have a 'DEFAULT_VALUE_FOR_BOOLEAN' for the static behavior? Similar to https://github.com/firebase/firebase-js-sdk/blob/master/packages/remote-config/src/value.ts#L36-L39?
There was a problem hiding this comment.
I agree it's inconsistent, but given the default value is an empty string when the source is static, BOOLEAN_TRUTHY_VALUES.indexOf(this.value.toLowerCase()) will return false, so I'm not seeing a technical need ... That said, I can imagine how explicitly defining the default value for boolean would make our system easier to understand, so I'll make the change.
| * @internal | ||
| */ | ||
| export class ValueImpl implements Value { | ||
| public static BOOLEAN_TRUTHY_VALUES = ['1', 'true', 't', 'yes', 'y', 'on']; |
There was a problem hiding this comment.
should we mark these as readonly?
There was a problem hiding this comment.
Makes sense. Will do.
| /** | ||
| * Defines the format for in-app default parameter values. | ||
| */ | ||
| export type DefaultConfig = { [key: string]: string | number | boolean }; |
There was a problem hiding this comment.
Could this also contain object or Value?
There was a problem hiding this comment.
We could add object. I think it'd pair well with a getObject<T>, so those could be the same review. I think it'd be safe to add these after the initial launch.
I don't think Value would make sense because it's a tuple of the string value and an indication of where it came from (the "source"). If the customer is setting it, then the its source will always be "default". Setting a string value in defaultConfig accomplishes the same thing in a more user-friendly way.
Per discussion on the PR, explicitly defining the default boolean value is more readable.
Discussion
RC's existing SDKs define type-specific getters, like
getBoolean. These aren't idiomatic for TS/JS, but have a couple advantages:This change defines an "internal" src directory for the new
ValueImplclass, since we now have three internal classes. We can move the other two in there in a follow-up change.Testing
Unit tested via
npm test. Functionally tested using a test server.API Changes
Updates the return type of the
evaluatemethod to be a rich object, modeled after the getters on the Web SDK.