Repository navigation
chore(remote-config): deprecate RemoteConfigFetchResponse and FetchResponseData #3302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -704,8 +704,14 @@ class VersionImpl implements Version { | |||||||||||||||||||||||||||||||||
| const HTTP_NOT_MODIFIED = 304; | ||||||||||||||||||||||||||||||||||
| const HTTP_OK = 200; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| let fetchResponseDeprecationWarned = false; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Represents a fetch response that can be used to interact with RC's client SDK. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * @deprecated Remote Config server-side rendering (SSR) hydration of the | ||||||||||||||||||||||||||||||||||
| * client SDK is no longer supported. This class will be removed in the | ||||||||||||||||||||||||||||||||||
| * next major release. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| export class RemoteConfigFetchResponse { | ||||||||||||||||||||||||||||||||||
| private response: FetchResponseData; | ||||||||||||||||||||||||||||||||||
|
|
@@ -714,8 +720,21 @@ export class RemoteConfigFetchResponse { | |||||||||||||||||||||||||||||||||
| * @param app - The app for this RemoteConfig service. | ||||||||||||||||||||||||||||||||||
| * @param serverConfig - The server config for which to generate a fetch response. | ||||||||||||||||||||||||||||||||||
| * @param requestEtag - A request eTag with which to compare the current response. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * @deprecated Remote Config server-side rendering (SSR) hydration of the | ||||||||||||||||||||||||||||||||||
| * client SDK is no longer supported. This class will be removed in the | ||||||||||||||||||||||||||||||||||
| * next major release. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| constructor(app: App, serverConfig: ServerConfig, requestEtag?: string) { | ||||||||||||||||||||||||||||||||||
| 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.' | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+729
to
+736
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the constructor to use the class's static property
Suggested change
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const config: { [key: string]: string } = {}; | ||||||||||||||||||||||||||||||||||
| for (const [param, value] of Object.entries(serverConfig.getAll())) { | ||||||||||||||||||||||||||||||||||
| config[param] = value.asString(); | ||||||||||||||||||||||||||||||||||
|
|
@@ -740,6 +759,10 @@ export class RemoteConfigFetchResponse { | |||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * @returns JSON representation of the fetch response that can be consumed | ||||||||||||||||||||||||||||||||||
| * by the RC client SDK. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * @deprecated Remote Config server-side rendering (SSR) hydration of the | ||||||||||||||||||||||||||||||||||
| * client SDK is no longer supported. This method will be removed in the | ||||||||||||||||||||||||||||||||||
| * next major release. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| public toJSON(): FetchResponseData { | ||||||||||||||||||||||||||||||||||
| return this.response; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1585,6 +1585,27 @@ describe('RemoteConfig', () => { | |||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| describe('RemoteConfigFetchResponse', () => { | ||||||||||||||||
| let warnStub: sinon.SinonStub; | ||||||||||||||||
|
|
||||||||||||||||
| beforeEach(() => { | ||||||||||||||||
| warnStub = sinon.stub(console, 'warn'); | ||||||||||||||||
| }); | ||||||||||||||||
|
Comment on lines
+1590
to
+1592
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reset the static
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| afterEach(() => { | ||||||||||||||||
| warnStub.restore(); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| // This test must run before any other construction of RemoteConfigFetchResponse | ||||||||||||||||
| // in this process, since the deprecation warning is emitted only once per process. | ||||||||||||||||
| it('should log a deprecation warning only once', () => { | ||||||||||||||||
| const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; | ||||||||||||||||
| const template = remoteConfig.initServerTemplate({ template: templateData }); | ||||||||||||||||
| new RemoteConfigFetchResponse(mockApp, template.evaluate()); | ||||||||||||||||
| new RemoteConfigFetchResponse(mockApp, template.evaluate()); | ||||||||||||||||
| expect(warnStub).to.have.been.calledOnce; | ||||||||||||||||
| expect(warnStub.firstCall.args[0]).to.include('RemoteConfigFetchResponse is deprecated'); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| it('should return a 200 response when supplied with no etag', () => { | ||||||||||||||||
| const templateData = deepCopy(SERVER_REMOTE_CONFIG_RESPONSE) as ServerTemplateData; | ||||||||||||||||
| // Defines remote parameter values. | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a global module-level variable
fetchResponseDeprecationWarnedto track whether the deprecation warning has been logged makes the unit tests order-dependent and fragile. If other tests instantiateRemoteConfigFetchResponsebefore the deprecation test runs, or if a single test is run in isolation (e.g., usingit.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
RemoteConfigFetchResponseclass and reset it in the test suite'sbeforeEachhook.