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

Change Error to FirestoreError #3418

Merged
merged 13 commits into from
Sep 9, 2020
Merged

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 16, 2020

Change the signature of onSnapshot() and onSnapshotsInSync() methods to report FirestoreError instead of a generic Error. This allows users to extract the error code from the error without having to do a questionable cast.

This will address #1515 for Firestore. A separate PR will be needed for Storage.

@changeset-bot
Copy link

changeset-bot bot commented Jul 16, 2020

🦋 Changeset is good to go

Latest commit: 7594809

We got this.

This PR includes changesets to release 9 packages
Name Type
@firebase/firestore Minor
@firebase/firestore-types Minor
firebase Patch
firebase-exp Patch
firebase-firestore-integration-test Patch
@firebase/rules-unit-testing Patch
@firebase/testing Patch
firebase-namespace-integration-test Patch
firebase-messaging-integration-test 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

@dconeybe dconeybe self-assigned this Jul 16, 2020
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 16, 2020

Binary Size Report

Affected SDKs

No changes between base commit (f47f990) and head commit (5abf4fc).

Test Logs

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

I looked at the callsites that you changed and verified that we only call them with a FirestoreError Everything else would have been a huge surprise. Please obtain API council approval before merging and add a Changeset.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 26, 2020

Size Analysis Report

Affected Products

No changes between base commit (f47f990) and head commit (5abf4fc).

Test Logs

@github-actions
Copy link
Contributor

Changeset File Check ⚠️

Warning: This PR modifies files in the following packages but they have not been included in the changeset file:

  • firebase

Make sure this was intentional.

@dconeybe dconeybe marked this pull request as ready for review September 9, 2020 17:01
@dconeybe dconeybe removed their assignment Sep 9, 2020
@dconeybe
Copy link
Contributor Author

dconeybe commented Sep 9, 2020

I'm going to merge despite the "Test All Packages" check failing. Its failure is due to a timeout and is not critical to this feature.

@dconeybe dconeybe merged commit a8ff3db into master Sep 9, 2020
@dconeybe dconeybe deleted the dconeybe/ErrorToFirestoreError branch September 9, 2020 17:20
@google-oss-bot google-oss-bot mentioned this pull request Sep 15, 2020
@firebase firebase locked and limited conversation to collaborators Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants