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

Update to Firebase Modular SDK #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

lemasc
Copy link

@lemasc lemasc commented Jun 9, 2021

First, thank you for this amazing library!

I've updated the firebase SDK to version 9 (beta) and changed any necessary hooks to make it works.
I also updated all dependencies to the latest version, but from my testing so far I still not found any breaking changes.

New SDK Type Errors

  1. Most of the new Firestore functions now not accepting options as undefined. Can it be simplify than this?

Now I'm using something like:

image

  1. There's a change with Firebase "SetOptions" type which causing type error. Now using //@ts-ignore

Here's the new definition

image

And now it showing me that

image

Any comments are appreciate. Actually I'm still new and learning typescript. If there's any change please reply me.

@nandorojo
Copy link
Owner

FWIW, I can add notes here that I see and help. However, as I mentioned on a separate issue, I'm no longer dedicated to this project and am looking for a maintainer.

As for the type issues: I think this PR would need to represent a major version bump. This means only supporting Firebase v9. So just use the normal types from firebase v9, and ignore the other ones.

@nandorojo
Copy link
Owner

nandorojo commented Jun 9, 2021

As for the options?.merge issue.

One option is to use a type guard.

function isMerge(options: SetOptions): options is MergeType {
  return !!(options as MergeType)?.merge
}
- if (options?.merge)
+ if (isMerge(options))

I typed that on my phone so apologies if there's a formatting issue

But reading the intent of the code, it should probably be called shouldMerge and it should check for both merge and mergeFields

@lemasc
Copy link
Author

lemasc commented Jun 9, 2021

@nandorojo Looks perfect! Thank you very much.

FWIW, I can add notes here that I see and help. However, as I mentioned on a separate issue, I'm no longer dedicated to this project and am looking for a maintainer.

I understand the situation. I think I can help this repo, but for only a period of time. When the COVID-19 pandemic ends, I need to go back to study. However I'm happy to do it anyway 😆.

@lemasc lemasc deleted the branch nandorojo:master October 9, 2021 01:14
@lemasc lemasc closed this Oct 9, 2021
@lemasc lemasc deleted the master branch October 9, 2021 01:14
@lemasc lemasc restored the master branch October 9, 2021 01:16
@lemasc lemasc reopened this Oct 9, 2021
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.

2 participants