Adds support for exporting a serialized config response from the RC server side SDK.#2829
Adds support for exporting a serialized config response from the RC server side SDK.#2829
Conversation
kjelko
changed the title
src/remote-config/remote-config.ts
Outdated
|
|
||
| if (currentEtag === requestEtag) { | ||
| this.response = { | ||
| status: 304, |
There was a problem hiding this comment.
Optional: replace the number with a readable constant?
| }); | ||
|
|
||
| describe('RemoteConfigFetchResponse', () => { | ||
| it('should return a 200 response with no etag', () => { |
There was a problem hiding this comment.
I was thrown off by the description, can we rephrase it as - "should return a 200 response when supplied with no etag"? Similarly for the next test.
| /** | ||
| * @param app - The app for this RemoteConfig service. | ||
| * @param serverConfig - The server config for which to generate a fetch response. | ||
| * @param requestEtag - A request eTag with which to compare the current response. |
There was a problem hiding this comment.
What's the request in this context? Do we expect the client to pass an etag to the customer server which can be used to avoid a config pushdown?
There was a problem hiding this comment.
That's correct - this would come in from a client request. it should be the etag of the most recent config that the client has. Since our internal servers do this, it makes sense to allow customer servers to do it as well
| /** | ||
| * @returns JSON representation of the fetch response that can be consumed | ||
| * by the RC client SDK. | ||
| */ |
There was a problem hiding this comment.
Probably needs to be annotated with @public to show up in the API docs?
There was a problem hiding this comment.
Hmm, I don't actually know - none of the other public methods are annotated with an @public...
| for (let i = 0; i < configJson.length; i++) { | ||
| const char = configJson.charCodeAt(i); | ||
| hash = (hash << 5) - hash + char; | ||
| hash |= 0; |
There was a problem hiding this comment.
What is the significance of this operation?
There was a problem hiding this comment.
Added a comment per below comment
| let hash = 0; | ||
| for (let i = 0; i < configJson.length; i++) { | ||
| const char = configJson.charCodeAt(i); | ||
| hash = (hash << 5) - hash + char; |
There was a problem hiding this comment.
This is quite cool. Should we add a comment describing the equation and also that we're trying to replicate the String.hashcode() impl here for context? I see that the java hashcode implementation mentions it.
…erver side SDK. (#2829) * add serialization method to server side remote config * update with latest changes * restructure as a class, add documentation * more tests for server config * run apidocs * Refine api and rerun docs * add back the newline * Apply suggestions from review * Make eTag optional in fetch response * rerun doc generator --------- Co-authored-by: Kevin Elko <kjelko@google.com>
5 participants
Adds support for exporting a serialized config response from the RC server side SDK.