Skip to content
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

Fix RTDB typings used by compat #7422

Merged
merged 6 commits into from
Jul 6, 2023
Merged

Fix RTDB typings used by compat #7422

merged 6 commits into from
Jul 6, 2023

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Jul 6, 2023

Note: Firestore changes will be done in a separate PR: #7423

E2E tests failed TypeScript compat build:
https://github.com/firebase/firebase-js-sdk/actions/runs/5470942751

due to changes on the following PRs:

Firestore PR: #7310
RTDB PR: #6541

The E2E tests install the staged NPM version of the firebase package and run some smoke tests. They failed on TypeScript compilation due to the following reasons:

1) For Firestore, types were only updated in Firestore modular, which is expected, as we don't add new features to compat. However, compat wraps modular classes, and it ran into an error importing their types and then checking them against firestore-types, which it uses as its types package. I'm not sure why it uses firestore-types instead of importing types directly from @firebase/firestore but that would be a future change. I did try it, but it's not a 1-to-1 correspondence, especially the FirebaseFirestore instance type, which doesn't exist in modular.

  1. For RTDB it's simpler, there was just a new type added as a param to firebase/compat/index.d.ts but not defined there.

@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2023

🦋 Changeset detected

Latest commit: 67dd5b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hsubox76 hsubox76 marked this pull request as ready for review July 6, 2023 03:56
@hsubox76 hsubox76 requested review from a team and dwyfrequency as code owners July 6, 2023 03:56
@hsubox76 hsubox76 requested review from dconeybe and maneesht July 6, 2023 03:56
Copy link
Contributor

@maneesht maneesht left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching this!

@google-oss-bot
Copy link
Contributor

@google-oss-bot
Copy link
Contributor

Copy link
Contributor

@dwyfrequency dwyfrequency left a comment

Choose a reason for hiding this comment

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

LGTM

@dconeybe dconeybe changed the title Fix Firestore and RTDB typings used by compat Fix RTDB typings used by compat Jul 6, 2023
@hsubox76 hsubox76 merged commit f3067f7 into master Jul 6, 2023
@hsubox76 hsubox76 deleted the ch-fix-firestore branch July 6, 2023 21:17
@google-oss-bot google-oss-bot mentioned this pull request Jul 6, 2023
@firebase firebase locked and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants