Get remote config template by version number #874
Conversation
| public getTemplate(): Promise<RemoteConfigTemplate> { | ||
| public getTemplate(versionNumber?: number | string): Promise<RemoteConfigTemplate> { | ||
| let path = 'remoteConfig'; | ||
| if (versionNumber) { |
There was a problem hiding this comment.
If the versionNumber is an invalid string (NaN) the backend API will return an error similar to the following:
errorInfo:
{ code: 'remote-config/invalid-argument',
message: 'Invalid value at \'version_number\' (TYPE_INT64), "abc"' },
codePrefix: 'remote-config' }
We could add a check here before making the request, but converting a string to a number to check isNaN will be tricky as the string is in int64 format. Appreciate any thoughts on this! :)
There was a problem hiding this comment.
We could also run a RegEx to check if the string only contains numbers @hiranya911
There was a problem hiding this comment.
How about:
const version = validators.isNumber(versionNumber) ? versionNumber : parseFloat(versionNumber);
if (!Number.isInteger(version)) {
throw
}
There was a problem hiding this comment.
Thanks! I am fine with this. If the string prefixes with numbers and followed by characters, parseFloat ignores the characters and parses the number in string.
ex: 70abc becomes 70
I believe this is okay, unless we would like further validate with a regex
| public getTemplate(): Promise<RemoteConfigTemplate> { | ||
| public getTemplate(versionNumber?: number | string): Promise<RemoteConfigTemplate> { | ||
| let path = 'remoteConfig'; | ||
| if (versionNumber) { |
There was a problem hiding this comment.
How about:
const version = validators.isNumber(versionNumber) ? versionNumber : parseFloat(versionNumber);
if (!Number.isInteger(version)) {
throw
}
| return this.getUrl() | ||
| .then((url) => { | ||
| const request: HttpRequestConfig = { | ||
| method: 'GET', | ||
| url: `${url}/remoteConfig`, | ||
| url: `${url}/${path}`, |
There was a problem hiding this comment.
Instead of appending to URL, try passing:
data: {versionNumber: `${version}`},
|
Updated and added a new test case for version number validation |
| let path = 'remoteConfig'; | ||
| if (versionNumber) { | ||
| path += '?versionNumber=' + versionNumber; | ||
| let requestData: any; |
| headers: FIREBASE_REMOTE_CONFIG_HEADERS | ||
| url: `${url}/remoteConfig`, | ||
| headers: FIREBASE_REMOTE_CONFIG_HEADERS, | ||
| ...requestData |
There was a problem hiding this comment.
This is not very readable. Change to data: requestData once the above changes are made.
There was a problem hiding this comment.
I used the spread syntax here to avoid sending an empty object for data when no version number is provided. Will update the code to be more readable :)
| let requestData: any; | ||
| if (typeof versionNumber !== 'undefined') { | ||
| this.validateVersionNumber(versionNumber); | ||
| requestData = { versionNumber: `${versionNumber}` }; |
There was a problem hiding this comment.
requestData['versionNumber'] = `${versionNumber}`
| 'versionNumber must be a non-empty string in int64 format or a number'); | ||
| } | ||
| const version = validator.isNumber(versionNumber) ? | ||
| versionNumber : parseFloat(versionNumber as string); |
There was a problem hiding this comment.
It looks like Number(str) provides stricter parsing.
There was a problem hiding this comment.
Number is 64 bit double (2^53 - 1) so I was concerned at first as we cannot represent int 64 (2^63 - 1). But it looks like Number.isInteger(Number(version)) should work here.
|
Hi @egilmorez! Please review the docs for the new changes. Thanks! |
src/index.d.ts
Outdated
| * | ||
| * | ||
| * @param versionNumber Version number of the Remote Config template to look up. | ||
| * If not specified, the latest Remote Config template will be returned. Optional. |
There was a problem hiding this comment.
Always prefer "is" to "will be" if it works. It does here! :)
There was a problem hiding this comment.
Good catch! Thank you. Updated!
* Get remote config template by version number (#874) * Get remote config template by version number * Refactor unit tests for Remote Config (#884) * Refactor unit tests * Add Remote Config Rollback operation (#885) * Add Remote Config Rollback operation * Update docs and move etag validation to a helper function * Update docs * Introduce a util to create a template from API response * PR fixes * Remote Config Add list versions operation (#896) * Remote Config: Add list versions operation * Add version Impl and other PR fixes * PR fixes * Imrpoved unit tests and some code clean up * Fix code formatting * Add a separate function to get a Template with version (#902) * Add version meta data to RC templates (#906) * Add version meta data to RC templates * PR fixes * Use assertion to unwrap template.version * Update Remote Config Docstrings in index.d.ts
NOTE: API proposal is still under review. Getting a head start