Skip to content

Define type-specific getters for SSRC #2519

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Apr 4, 2024
Merged

Define type-specific getters for SSRC #2519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5732ea9
Update SDK to use type-specific getters
erikeldridge Mar 30, 2024
a59da2f
Stringify default config on construction
erikeldridge Mar 30, 2024
9bb2303
Move Value instantiation to evaluation stage
erikeldridge Mar 30, 2024
fbecfe7
Remove references to valueType and description from tests
erikeldridge Apr 2, 2024
a4b78f3
Define unit tests for Value
erikeldridge Apr 2, 2024
b6bbe9d
Define type for default config
erikeldridge Apr 2, 2024
f2509b4
Extract API
erikeldridge Apr 2, 2024
f0d4e53
Add copyright header to new file
erikeldridge Apr 2, 2024
9e0cb44
Make boolean getter case-insensitive
erikeldridge Apr 2, 2024
fac1259
Merge remote-tracking branch 'gh-public/ssrc' into ssrc-getters
erikeldridge Apr 2, 2024
a28252c
Make Value constants readonly and define default boolean value.
erikeldridge Apr 4, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make boolean getter case-insensitive
  • Loading branch information
@erikeldridge
erikeldridge committed Apr 2, 2024
commit 9e0cb44ea80fbbf3a01e68effda5f95e8cbc0b56
4 changes: 2 additions & 2 deletions src/remote-config/internal/value-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class ValueImpl implements Value {
private readonly source: ValueSource,
private readonly value = ValueImpl.DEFAULT_VALUE_FOR_STRING) { }
asBoolean(): boolean {
Copy link

@trekforever trekforever Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Contributor Author

@erikeldridge erikeldridge Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value) >= 0;
return ValueImpl.BOOLEAN_TRUTHY_VALUES.indexOf(this.value.toLowerCase()) >= 0;
}
asNumber(): number {
const num = Number(this.value);
Expand All @@ -51,4 +51,4 @@ export class ValueImpl implements Value {
getSource(): ValueSource {
return this.source;
}
}
}
2 changes: 1 addition & 1 deletion src/remote-config/remote-config-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,4 +633,4 @@ export type ValueSource = 'static' | 'default' | 'remote';
/**
* Defines the format for in-app default parameter values.
*/
export type DefaultConfig = { [key: string]: string | number | boolean };
export type DefaultConfig = { [key: string]: string | number | boolean };
Copy link
Member

@lahirumaramba lahirumaramba Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also contain object or Value?
Copy link
Contributor Author

@erikeldridge erikeldridge Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
9 changes: 7 additions & 2 deletions test/unit/remote-config/internal/value-impl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,19 @@ describe('ValueImpl', () => {
});

describe('asBoolean', () => {
it('returns true for truthy values', () => {
it("returns true for any value in RC's list of truthy values", () => {
for (const truthyValue of ValueImpl.BOOLEAN_TRUTHY_VALUES) {
const value = new ValueImpl('default', truthyValue);
expect(value.asBoolean()).to.be.true;
}
});

it('returns false for falsy values', () => {
it('is case-insensitive', () => {
const value = new ValueImpl('default', 'TRUE');
expect(value.asBoolean()).to.be.true;
});

it("returns false for any value not in RC's list of truthy values", () => {
const value = new ValueImpl('default', "I'm falsy");
expect(value.asBoolean()).to.be.false;
});
Expand Down