Skip to content

Conversation

lahirumaramba
Copy link
Member

  • Implement getTemplate()
  • Introduce new RemoteConfigParameterValue type
  • To closely represent the backend REST API, change parameters in RemoteConfigTemplate to a hashmap
  • Add unit tests for getTemplate

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Just bunch of nits and suggestions to improve on.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Let's rename to etag as indicated, and tighten up the tests.

readonly conditions?: RemoteConfigCondition[];
readonly parameters?: { [key: string]: RemoteConfigParameter };
readonly version?: Version; //only undefined when there is no active template in the project
readonly eTag: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the backend already sends this as etag. It's quite common to write this as etag. You will find many instances of that in that same page you've referenced. But it's almost never written as eTag in my experience.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

'invalid-argument',
'ETag header is not present in the server response.');
}
const remoteConfigResponse: RemoteConfigResponse = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You can collapse this into one statement.

return {
  ...
};

@lahirumaramba lahirumaramba merged commit 14e2920 into remote-config Mar 6, 2020
@lahirumaramba lahirumaramba deleted the lm-rc-get-validate branch March 6, 2020 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants