Repository navigation
change(rc): deprecate RemoteConfigFetchResponse and FetchResponseData - #3302
ritiktaneja wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the FetchResponseData interface and the RemoteConfigFetchResponse class in the Remote Config module, adding deprecation warnings and corresponding unit tests. The feedback points out that using a global module-level variable to track the warning state makes unit tests order-dependent and fragile. It is recommended to refactor this state into a static property on the RemoteConfigFetchResponse class and reset it in the test suite's beforeEach hook to ensure test isolation.
| beforeEach(() => { | ||
| warnStub = sinon.stub(console, 'warn'); | ||
| }); |
There was a problem hiding this comment.
Reset the static deprecationWarned property on RemoteConfigFetchResponse before each test to ensure that tests are completely isolated and independent of execution order.
| beforeEach(() => { | |
| warnStub = sinon.stub(console, 'warn'); | |
| }); | |
| beforeEach(() => { | |
| warnStub = sinon.stub(console, 'warn'); | |
| (RemoteConfigFetchResponse as any).deprecationWarned = false; | |
| }); |
7f45b94 to
50b56ea
Compare
tusharkhandelwal8
left a comment
tusharkhandelwal8
left a comment
There was a problem hiding this comment.
Approved, but apply the suggested changes if required.
| beforeEach(() => { | ||
| warnStub = sinon.stub(console, 'warn'); | ||
| }); |
…sponseData Remote Config server-side rendering (SSR) hydration of the client SDK has been deprecated; the backend rejects client fetches of server templates, and the JS SDK is deprecating templateId/initialFetchResponse in parallel. - Mark RemoteConfigFetchResponse and FetchResponseData @deprecated (to be removed in the next major release). - Log a one-time console warning when RemoteConfigFetchResponse is constructed. - No changes to getServerTemplate / initServerTemplate / evaluate().
…ng flag Moves the one-time warning flag from module scope to a private static property on RemoteConfigFetchResponse and resets it in the test's beforeEach, so the warning test no longer depends on execution order.
17e4c6a to
4abf33e
Compare
lahirumaramba
left a comment
lahirumaramba
left a comment
There was a problem hiding this comment.
Thanks @ritiktaneja! Overall this looks good. Added a couple of quick questions and a minor nit.
| * | ||
| * @deprecated Remote Config server-side rendering (SSR) hydration of the | ||
| * client SDK is no longer supported. This type will be removed in the | ||
| * next major release. |
There was a problem hiding this comment.
Could we get a TW review on these new deprecation docstrings, and would it help to mention an alternative or link a migration guide if one exists?
There was a problem hiding this comment.
@nk-1983 can I ask you to review this whenever you're available?
lahirumaramba
changed the title
Remote Config server-side rendering (SSR) hydration of the client SDK has been deprecated; the backend rejects client fetches of server templates, and the JS SDK is deprecating templateId/initialFetchResponse in parallel.