Repository navigation
chore(remote-config): deprecate RemoteConfigFetchResponse and FetchResponseData - #3302
ritiktaneja wants to merge 1 commit into
Conversation
…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().
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.
| const HTTP_NOT_MODIFIED = 304; | ||
| const HTTP_OK = 200; | ||
|
|
||
| let fetchResponseDeprecationWarned = false; |
There was a problem hiding this comment.
Using a global module-level variable fetchResponseDeprecationWarned to track whether the deprecation warning has been logged makes the unit tests order-dependent and fragile. If other tests instantiate RemoteConfigFetchResponse before the deprecation test runs, or if a single test is run in isolation (e.g., using it.only), the deprecation warning test will fail because the warning will not be logged again.
To make the tests robust and independent, we can store this state as a private static property on the RemoteConfigFetchResponse class and reset it in the test suite's beforeEach hook.
| if (!fetchResponseDeprecationWarned) { | ||
| fetchResponseDeprecationWarned = true; | ||
| console.warn( | ||
| 'RemoteConfigFetchResponse is deprecated and will be removed in the next major ' + | ||
| 'release. Remote Config server-side rendering (SSR) hydration of the client SDK ' + | ||
| 'is no longer supported.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Update the constructor to use the class's static property RemoteConfigFetchResponse.deprecationWarned instead of the global module-level variable. This allows the warning state to be reset between tests. Note that you should also add private static deprecationWarned = false; inside the RemoteConfigFetchResponse class definition.
| if (!fetchResponseDeprecationWarned) { | |
| fetchResponseDeprecationWarned = true; | |
| console.warn( | |
| 'RemoteConfigFetchResponse is deprecated and will be removed in the next major ' + | |
| 'release. Remote Config server-side rendering (SSR) hydration of the client SDK ' + | |
| 'is no longer supported.' | |
| ); | |
| } | |
| if (!RemoteConfigFetchResponse.deprecationWarned) { | |
| RemoteConfigFetchResponse.deprecationWarned = true; | |
| console.warn( | |
| 'RemoteConfigFetchResponse is deprecated and will be removed in the next major ' + | |
| 'release. Remote Config server-side rendering (SSR) hydration of the client SDK ' + | |
| 'is no longer supported.' | |
| ); | |
| } |
| 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; | |
| }); |
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.