Skip to content
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

feat(rc): Add server side Remote Config support #2529

Merged
merged 17 commits into from
Apr 15, 2024
Merged

feat(rc): Add server side Remote Config support #2529

merged 17 commits into from
Apr 15, 2024

Conversation

erikeldridge
Copy link
Contributor

Discussion

We've been merging SSRC changes into the ssrc integration branch. This PR merges ssrc into master.

Testing

Unit tested by running npm test.

Functionally tested by installing the Admin SDK in a test server using this branch.

API Changes

Incrementally reviewed via ssrc-prefixed feature branches.

erikeldridge and others added 14 commits February 14, 2024 11:24
Define SSRC API
---------

Co-authored-by: Xin Wei <trekforever@users.noreply.github.com>
Add API changes needed for SSRC

---------

Co-authored-by: Xin Wei <xinwei@google.com>
Co-authored-by: jen_h <harveyjen@google.com>
Add public SSRC methods

---------

Co-authored-by: Xin Wei <xinwei@google.com>
Co-authored-by: jen_h <harveyjen@google.com>
We have a Firebase AIP to avoid these prefixes.
…ith defaults. (#2503)

In the logic where we backfill config with defaults, the first argument to Object.assign should be an object to assign to, but the code passed the object containing the defaults.
In practice, we only set default config using the initialization
methods, so make the internal field private to simplify the API.
* Get And condition passing

* Support and.or condition

* Support true condition

* Support false condition, and fix tests

* Use or.and, not the other way around

* Integrate conditional values into evaluate method

* Test handling for multiple conditions

* Clean up logs

* Extract condition evaluation to class for testing

* Namespace condition names

* Iterate over ordered condition list

* Test condition ordering

* Differentiate named conditions

* Document condition types

* Generalize condition eval test and fix styling

* Replace log statement with todo

* Implement evaluate percent condition for RC server-side

* Apply lint fixes

* Add context param to evaluate method

* Add tests for percent condition eval

* Update evaluator tests to use context

* Increase threshold to +/- 500 for percent condition eval tests
to prevent some flaky tests.

* Clean up percentCondition tests a bit and add note on the tolerance used

* Apply suggestions from code review

Co-authored-by: jen_h <harveyjen@google.com>

* Update copyright date and remove stray log statement

* Mock farmhash in tests

* Add Math.abs for farmhash - to be consistent with the internal implementation

* Regenerate package-lock to fix Node 14 CI error re busboy

* Fix lint errors

* Rename "id" to "randomizationId" per discussion

* Extract API

* Only return false in cases of uknown template evaluation

* Remove product prefix from type names

* Remove product prefix from exported types

* Remove unused "expression" field from server condition

* Extract API

* Remove prefix from impl classes, for consistency

* Remove prefix from new internal classes

* Remove "server" prefix

* Remove prefix from NamedCondition

* Rename "or" and "and" fields to match API

* Rename "operator" field to "percentOperator" to match API

* Extract API after "and" and "or" rename

* use longjs library for hash

* re-run npm install

* re-attempt

* use node 14, re-attempt

* remove file

* Add comment, switch from lte to lt

---------

Co-authored-by: Erik Eldridge <erikeldridge@google.com>
Co-authored-by: Xin Wei <xinwei@google.com>
Co-authored-by: jen_h <harveyjen@google.com>
…a RC server template (#2520)

* Add support to pass in a json string for RC server template initialization

* Merge ssrc changes

* Apply lint changes

* Update comments

* Run api-extractor:local

* Update comments and address feedback

* Update inline comment and lint fixes

* Revert toJSON functionality for ServerTemplate
RC's existing SDKs define type-specific getters, like getBoolean.
These aren't idiomatic for TS/JS, but have a couple advantages:

1. RC param names, values and types are mutable remotely, so a
simple object can’t guarantee a strict type for application logic. A
formal schema would address this, but feels excessive for the
common case. Type-specific methods are consistent with RC's
current SDKs and ensure appropriate types for application logic.
2. RC Android and iOS SDKs log events when personalized values
are used. A method interface facilitates such additional functionality
* Add #set on ServerTemplate to allow for setting and caching a server template

* Simplify initServerTemplate to make use of the new setter

* Update tests

* Address comments and feedback

* Update API docs

* Add export for new type ServerTemplateDataType to index.ts

* Update some inline comments

Choose a reason for hiding this comment

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

nit: since we have a new internal folder now (currently only containing value-impl.ts), would we want to move this file into there and rename it to just condition-evaluator.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but I think we can safely do that in a follow-up change, since it's internal.

I also just noticed that we use the @internal annotation on the RemoteConfig constructor, which makes me wonder if the convention should just be that. Anything with the annotation is internal, regardless of the file's name or location. We could then remove the "internal" directory and file name suffixes.

public readonly etag: string;
public version?: Version;

constructor(template: ServerTemplateData) {

Choose a reason for hiding this comment

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

nit: template can be readonly too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd make sense, but I'm unsure it's worth another PR. The current approach is comparable to the existing RemoteConfigTemplateImpl constructor.

Aside, this reminds me of java's final. Most things are readonly, but applying a modifier to each variable ends up being more trouble than it's worth. A single designation, like const, is better.

@lahirumaramba lahirumaramba changed the title Merges SSRC into master feat(rc): Add server side Remote Config support Apr 15, 2024
@lahirumaramba lahirumaramba added release-note release:stage Stage a release candidate labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:stage Stage a release candidate release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants