Add #set and #toJSON to RC ServerTemplate #2522
Conversation
| } else { | ||
| template.cache = options?.template; | ||
| } | ||
| template.set(options?.template); |
There was a problem hiding this comment.
Nice, intuitive reuse of set 👍
| const initializedTemplate = remoteConfig.initServerTemplate(); | ||
| initializedTemplate.set(template); | ||
| const parsed = initializedTemplate.toJSON(); | ||
| expect(parsed).deep.equals(template); |
There was a problem hiding this comment.
This is very readable 👍
| .to.throw(/Failed to parse the JSON string: ([\D\w]*)\./); | ||
| }); | ||
|
|
||
| INVALID_PARAMETERS.forEach((invalidParameter) => { |
There was a problem hiding this comment.
Thinking: it feels like we're re-testing the cases covered by the tests on ServerTemplateDataImpl. Looking at the tests, I see us already doing this on the tests for initServerTemplate and load. Makes me think there's an opportunity for a later change to extract ServerTemplateDataImpl to the internal dir so we can test it directly.
| * Sets and caches a {@link ServerTemplateData} or a JSON string representing | ||
| * the server template | ||
| */ | ||
| set(template: ServerTemplateDataType): void; |
There was a problem hiding this comment.
Just curious (because I think either is fine) so we need a separate type for this or should this just be ServerTemplateData | string here?
There was a problem hiding this comment.
I think either works, I originally had it as ServerTemplateData | string but we are defining it in quite a few places across multiple files, so I moved it to a type 😃
Discussion
Right now, we can set a RC server template via the constructor. Customers, however, would need to update the template multiple times. The current API exposes
template.cache, but that feels more like an internal detail than an API now.ServerTemplate.cacheasprivate, and should only be used with-in the classServerTemplate.setto allow for setting the template or templateJSONServerTemplate.toJSONto return JSON representation (ServerTemplateData) of the templateInitServerTemplateto make use of the new set method.Testing
Added new unit tests for the new
setmethod. Update existing tests to make use oftoJSON()instead of the cache.API Changes
ServerTemplate.cacheis now private and cannot be accessed outside of the classServerTemplate.setto allow for setting the template or templateJSONServerTemplate.toJSONto return JSON representation (ServerTemplateData) of the template