Skip to content

Set fdcrealtime flag to true by default and modify tests.#10279

Open
itsrakhil wants to merge 2 commits intofirebase:mainfrom
itsrakhil:main
Open

Set fdcrealtime flag to true by default and modify tests.#10279
itsrakhil wants to merge 2 commits intofirebase:mainfrom
itsrakhil:main

Conversation

@itsrakhil
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables the Firebase Data Connect realtime feature by default and updates the SDK generation logic to inject a clientCache configuration. The review feedback correctly identifies a violation of the repository's style guide regarding the use of 'any' as an escape hatch in unit tests, suggesting the use of Chai's nested property assertions instead.

it("should NOT inject clientCache if fdcrealtime is disabled", () => {
sinon.stub(experiments, "isEnabled").withArgs("fdcrealtime").returns(false);
addSdkGenerateToConnectorYaml(connectorInfo, connectorYaml, app);
expect((connectorYaml.generate?.javascriptSdk as any)[0].clientCache).to.be.undefined;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using as any to access properties in tests. This violates the repository style guide which discourages the use of any as an escape hatch. You can use Chai's nested property assertions to achieve the same result in a more readable manner without bypassing type safety via any.

Suggested change
expect((connectorYaml.generate?.javascriptSdk as any)[0].clientCache).to.be.undefined;
expect(connectorYaml.generate?.javascriptSdk).to.have.nested.property("[0].clientCache").that.is.undefined;
References
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

Copy link
Copy Markdown
Contributor

@fredzqm fredzqm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

Please double check with @aashishpatil-g on whether we should tweak the default client_cache configs yaml.

@itsrakhil
Copy link
Copy Markdown
Contributor Author

LG.

Please double check with @aashishpatil-g on whether we should tweak the default client_cache configs yaml.

Checked with @aashishpatil-g offline to confirm that the current setup is good.

@itsrakhil itsrakhil enabled auto-merge (rebase) April 7, 2026 22:50
@itsrakhil itsrakhil disabled auto-merge April 8, 2026 09:14
@itsrakhil itsrakhil closed this Apr 8, 2026
@itsrakhil itsrakhil reopened this Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants