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

Compatibility with Firebase Modular JS SDK (v9) #1128

Closed
sceee opened this issue Jul 5, 2021 · 33 comments
Closed

Compatibility with Firebase Modular JS SDK (v9) #1128

sceee opened this issue Jul 5, 2021 · 33 comments

Comments

@sceee
Copy link

sceee commented Jul 5, 2021

What problem is this solving

Firebase recently introduced the modular web SDK, currently in beta.
One of its goals is to reduce the footprint of the lib allowing tree shaking. Therefore, a lot of users might want to benefit from using it.

Using vuexfire does currently not seem to be working with the new Firebase modular SDK because of the changed API:

TypeError: document.onSnapshot is not a function
    at bindDocument (webpack-internal:///./node_modules/vuexfire/dist/vuexfire.esm.js:456)
    at eval (webpack-internal:///./node_modules/vuexfire/dist/vuexfire.esm.js:614)
    at new Promise (<anonymous>)
    at bind$1 (webpack-internal:///./node_modules/vuexfire/dist/vuexfire.esm.js:603)
    at bindFirestoreRef (webpack-internal:///./node_modules/vuexfire/dist/vuexfire.esm.js:655)

See here for the new way the API works. What used to be something like this:

db.collection("cities").doc("SF")
    .onSnapshot((doc) => {
        console.log("Current data: ", doc.data());
    });

...is now something like this:

import { doc, onSnapshot } from "firebase/firestore";

const unsub = onSnapshot(doc(db, "cities", "SF"), (doc) => {
    console.log("Current data: ", doc.data());
});

That's why I think the bindFirestoreRef(...) now needs to be adjusted to adjust to the new API.

Proposed solution

As version 9 of the Firebase JS SDK will be the modular one, adjust to the new API so that vuefire/vuexfire works with the Firebase 9 JS SDK.

Describe alternatives you've considered

Stick with Version 8 of the Firebase JS SDK for now.

@sceee
Copy link
Author

sceee commented Jul 8, 2021

@posva I would be happy to help with the migration to the modular Firebase 9 SDK but unfortunately, I don't really understand how to build the projects (I'm not really into lerna & rollup).

Is there anywhere some hint on how to build & test the whole repository?
If so I would be happy to create a PR.

@posva
Copy link
Member

posva commented Jul 8, 2021

@sceee the branch that needs to be updated is https://github.com/vuejs/vuefire/tree/v3 which is the next version of Vuefire that works for both Vue 2 and Vue 3. I haven't checked the breaking changes aside from modules from 8 to 9 but if that's the only breaking change, then it's fine to upgrade it.

Tests are currently failing after the upgrade to firebase 8 but it's no longer a lerna monorepo, so it should be easier to handle 🙂

@sceee
Copy link
Author

sceee commented Jul 8, 2021

@posva thank for the info - I will have a look and see if I can prepare a proposed upgrade.

I think there's not yet a changelog available for Firebase SDK v9 (as it's also still in Beta) but according to my information, this seems to be the only change.
There are some slight limitations at the moment, though, but according to my understanding they are not relevant to vuefire: https://firebase.google.com/docs/web/modular-upgrade#benefits

Anyway, I think the way forward will be v9 and therefore I think it would be good have vuefire support it better earlier than later (maybe also as a separate (alpha/beta) release so that Firebase v8 users can still use vuefire v3 and Firebase v9 users can use vuefire v4 (alpha/beta)?).
Would you agree?

@sceee
Copy link
Author

sceee commented Jul 8, 2021

I just noticed that firebase-mock is used for the tests which is already a blocker as it depends on the namespaced Firebase SDK (v8).

I anyway guess firebase-mock is kind of obsolete and it will probably not continue to be maintained well in the future as there is the official Firebase emulator suite now for local testing (which works way better IMHO).

So it would probably be better (and required) to migrate from firebase-mock to using the Firebase emulators for tests first.
Do you think this would make sense?

It unfortunately seems it's getting a bigger change than I initially thought...

@posva
Copy link
Member

posva commented Jul 8, 2021

Do you think this would make sense?

Yes, feel free to do it

@sceee
Copy link
Author

sceee commented Jul 9, 2021

I pushed some work in progress here: https://github.com/sceee/vuefire/tree/firebase-emulators
and created an Action that executes the tests here: https://github.com/sceee/vuefire/actions/workflows/test.yml

Still, a lot of tests are failing but it's getting better.
I hope I can get all of the more fancy tests to work with the new setup.

@sceee
Copy link
Author

sceee commented Jul 11, 2021

@posva I fixed some more tests (it's now down to ~20 failing) but now I'm getting to a point where I'm getting stuck.

I think there are a few groups of issues left:

  • Timing issues (target.items.value did not yet update after a item.set (DB update) call) - I introduced sleeps here which is definitely not ideal but solved a lot of them. Still, some are sometimes failing. I also discovered some floating promises in the code (but also production code). It could also result from these but I do not want to change the production code. Probably for more reliability, some timeouts need to be introduced/increased in the tests.
  • Sometimes (also seems to be a timing issue) the following exception occurs (which - based on the comment above that line - looks kind of like a bug to me). It seems this occurs if the data is not yet bound correctly:
   refs in collections  keeps array of references when updating a property

    TypeError: Cannot read property '0' of undefined

      21 | ): Record<string, any> {
      22 |   // TODO: development warning when target[key] does not exist
    > 23 |   return path.split('.').reduce((target, key) => target[key], obj)
         |                                                        ^
      24 | }
      25 |
      26 | /**

      at src/shared.ts:23:56
          at Array.reduce (<anonymous>)
      at Object.walkGet (src/shared.ts:23:26)
      at Object.data (src/firestore/index.ts:151:19)
  • some unbind spies do not work/are not triggered as expected. Example:
   refs in collections  unbinds refs when the collection is unbound

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      128 |     await delay(200)
      129 |
    > 130 |     expect(spyA).toHaveBeenCalledTimes(1)
          |                  ^
      131 |     expect(spyB).toHaveBeenCalledTimes(1)
      132 |
      133 |     spyA.mockRestore()

      at Object.<anonymous> (__tests__/core/firestore/refs-collections.spec.ts:130:18)

I have the feeling that behavior for the unbind changed in Firebase compared to how firebase-mock mocks it.
So I'm actually not sure if there's really an issue now or if the tests simply would need to be adjusted. <-- This is where I would appreciate some help

Besides that, I had to make some general adjustments to some tests:

  • Besides the timing issue, the order of elements when "storing" a bound collection in target.items.value is no longer deterministic. I have the feeling this is in-line with Firebase production service (it was just deterministic with firebase-mock) but I'm not 100% sure.
  • The amount of times some spies are called changed (increased)

Would you think you have the possibility to take a look at the remaining failing tests (especially the failing unbind tests) to assess whether they really need to work like they are at the moment or some can be adjusted to the "new" behavior? Or if you have an idea why some of them are failing/how to fix them?

If you want to execute the tests locally, please

  1. First install firebase-tools globally if you don't yet have them: npm i -g firebase-tools
  2. Execute npm run test:unit - You can also execute the emulators using firebase emulators:start from the project directory and then - in a separate terminal, execute the tests (or single files) using npm run test:unit:execute. Use npm run test:unit:execute -- --t core/firestore/util if you just want to execute some files / it.only tests

If you prefer, I could also create a PR (draft) so we move the future discussion over there so we don't spam this issue.

Happy to hear what you think.

@posva
Copy link
Member

posva commented Jul 13, 2021

I will take a proper look in a few weeks, after some Vue 3 related releases, and try to give you a hand. Have you published your code somewhere?

@sceee
Copy link
Author

sceee commented Jul 14, 2021

Thanks @posva . Yeah, the code is published here https://github.com/sceee/vuefire/tree/firebase-emulators and the latest action run always shows the up to date state.

@posva
Copy link
Member

posva commented Jul 19, 2021

Thanks a lot @sceee !

I will get to this probably after a stable release on Pinia with Vue 3.2

@dosstx
Copy link

dosstx commented Jul 26, 2021

Thank you both for the work on migrating to v9. Even though v9 is still beta, I am loving it. The API is fantastic and everything is much faster. Just wanted to say keep up the great work!

@bangdragon
Copy link

How is the progress. I would love to use this library for firebase v9 because it has tree shaking.
Thanks

@sceee
Copy link
Author

sceee commented Aug 27, 2021

@posva quick question - is the code in old_src in the v3 branch still used or can it be deleted (just to make things easier to overview)?

@posva
Copy link
Member

posva commented Aug 27, 2021

I think it can be ignored

@bangdragon
Copy link

Firebase v9 has been released, hope you guys release vuexfire soon. Thanks

@gabrielnvian
Copy link

What's the progress on this?

derat added a commit to derat/ascenso that referenced this issue Oct 26, 2021
I initially tried to go to [email protected], the current
release, but it redoes the way that module imports work. It
seems straightforward to update our code to use the 'compat'
versions of the modules per the instructions at
https://firebase.google.com/docs/web/modular-upgrade, but
the third-party vuefire package doesn't seem to have been
updated yet (vuejs/vuefire#1128)
so we're stuck at version 8 for now.

Also switch from the deprecated @firebase/testing package to
@firebase/rules-unit-testing. Luckily, it seems to be a
drop-in replacement.
derat added a commit to derat/ascenso that referenced this issue Nov 1, 2021
I initially tried to go to [email protected], the current
release, but it redoes the way that module imports work. It
seems straightforward to update our code to use the 'compat'
versions of the modules per the instructions at
https://firebase.google.com/docs/web/modular-upgrade, but
the third-party vuefire package doesn't seem to have been
updated yet (vuejs/vuefire#1128)
so we're stuck at version 8 for now.

Also switch from the deprecated @firebase/testing package to
@firebase/rules-unit-testing. Luckily, it seems to be a
drop-in replacement.

To get all of this working, I also ripped out all of the
code I'd written before to asynchronously import Firebase
modules. It made the code way more complicated for what was
probably only a little bit of improvement in loading time,
and I spent a lot of time trying to figure out how to update
it to work with version 8 of the Firebase API before giving
up. Version 9 finally supports tree-shaking, so hopefully
things will get faster again when we're able to upgrade to
it.
derat added a commit to derat/ascenso that referenced this issue Nov 1, 2021
I initially tried to go to [email protected], the current
release, but it redoes the way that module imports work. It
seems straightforward to update our code to use the 'compat'
versions of the modules per the instructions at
https://firebase.google.com/docs/web/modular-upgrade, but
the third-party vuefire package doesn't seem to have been
updated yet (vuejs/vuefire#1128)
so we're stuck at version 8 for now.

Also switch from the deprecated @firebase/testing package to
@firebase/rules-unit-testing. Luckily, it seems to be a
drop-in replacement.

To get all of this working, I also ripped out all of the
code I'd written before to asynchronously import Firebase
modules. It made the code way more complicated for what was
probably only a little bit of improvement in loading time,
and I spent a lot of time trying to figure out how to update
it to work with version 8 of the Firebase API before giving
up. Version 9 finally supports tree-shaking, so hopefully
things will get faster again when we're able to upgrade to
it.
@sceee
Copy link
Author

sceee commented Jan 19, 2022

To all asking/waiting for progress on this - I would really appreciate to get this working but unfortunately, I got stuck while making the tests work using the Firebase emulator as mentioned here.

@posva mentioned in some of the comments above he might be able to find some time to help check the remaining (currently failing) tests.
I will also try again to work again on this but as mentioned in the post above, I would appreciate some help to assess whether the failing tests are still required to work (or whether they depended on some mock internals with the previous Firebase Mock solution that now changed with the Firebase emulators).

Once all the tests work with Firebase 8 using the Firebase emulator, the migration to the v9 modular SDK should be a smaller step.

@sceee
Copy link
Author

sceee commented Feb 11, 2022

I further analyzed the remaining failing tests (roughly ~14-19).

Unfortunately, I could not get them to work but at least I now have a better idea what causes some of the failures:
Some of the remaining failing tests (try to) spy calls to the unsubscribe function returned by a onSnapshot listener attachment on Firebase references to assert that they are called under specific conditions.

To do this, spys are added in tests using spyUnbind to spy for calls to the unsubscribe function (see here https://github.com/vuejs/vuefire/blob/v3/__tests__/src/index.ts#L17). This spyUnbind function previously replaced the DocumentReference/CollectionReference onSnapshot method with an own implementation to intercept calls to the unsubscribe method and be able to "register" calls to this method.

Unfortunately, using the Firebase emulators, this code no longer works as before as the replacement onSnapshot never gets called. I guess that's because another DocumentReference is used which restores the "original" onSnapshot method.

The same issue probably applies to the delayUpdate function.

Update: I could see that spying on the unsubscribe function still works for direct references, so the following still works:

    const cSpy = spyUnbind(c)

    const unsubscribe = c.onSnapshot((doc) => {
      console.log(`onSnapshot called for test ${doc.ref.path}`)
    })

    expect(cSpy).toHaveBeenCalledTimes(0)

    unsubscribe()

    expect(cSpy).toHaveBeenCalledTimes(1)

But it no longer works for nested refs (DocumentData for doc a is { test: c } where c is a DocumentReference) as they will "keep" the original onSnapshot implementation without spy interception - the ref is "created" here and I did not find a way to intercept the onSnapshot for these "automatic" DocumentReferences:
https://github.com/vuejs/vuefire/blob/v3/src/firestore/utils.ts#L82

Does maybe someone have an idea how to solve this issue as I could not find a way to intercept these calls to be able to spy on them to make the tests work?

Besides that, there also seems to be some timing issue left in the code (maybe un-awaited promises) as some tests sometimes work and sometimes fail with errors like

TypeError: Cannot read property '0' of undefined

caused by the walkGet function where target in the reduce function splittedPath.reduce((target, key) => target[key], obj) is undefined.

@luc122c
Copy link
Contributor

luc122c commented Mar 1, 2022

Hi, I'm really looking forward to this. Vuefire looks like a fantastic solution, but since Vue3 / Vuex 4 / Firebase 9 are the latest stable versions, I'm not keen on adding it to current projects.

Do you have a rough estimate of how much work is left to complete?
How can the community help / What's the best way to test this for you?
Could a branch and/or npm tag be set up for this work?

@sceee
Copy link
Author

sceee commented Mar 2, 2022

@luc122c it's not so much left, see here: https://github.com/sceee/vuefire/runs/5158585563?check_suite_focus=true
Current state is something like this: Tests: 17 failed, 7 skipped, 135 passed, 159 total

Mainly this is due to tests failing cause the spyUnbind no longer works as it did in the past as I wrote in the previous comment.

I think once the tests work, migrating to the new Firebase 9 API is the easier task and should be pretty straightforward.

So if anyone has an idea how to get the spyUnbind work with the new Emulator testing, it would help.

@posva
Copy link
Member

posva commented Mar 2, 2022

Still needs to be confirmed but I should be able to spend time bringing v9 support to VueFire as well as support for other Firebase packages. Stay tuned! Thanks a lot for the help @sceee, I will keep you updated with an alternative way to spy on unsubscriptions as it would be important to be able to not use await delay(x).

@JuliusJacobitz
Copy link

Looking forward for an update on this topic :) Thanks for the great work!

@tmaly1980
Copy link

tmaly1980 commented May 2, 2022

Any progress on this in the last 2 months? Can someone please update the docs/install guide so the default vuejs/firebase installation doesn't automatically install an incompatible version, and be more version explicit?

@michi88
Copy link

michi88 commented May 28, 2022

How do we feel about making a release for firebase v9 using the firebase/compat import?

https://firebase.google.com/docs/web/modular-upgrade#update_imports_to_v9_compat

This would unblock v9 upgrades. Funny enough, even the google project firebaseui has done this.

I've been trying for a bit but unfortunately I'm not super skilled at all the npm workspace rollup yarn packaging stuff going on in this project. Would be great to have some docs on how to run this project as a lib developer.

If we would consider releasing a v9 compat version I can spend some more time on this. Thanks!

@michi88
Copy link

michi88 commented Jun 3, 2022

Made a branch here that can be installed like this in package.json:

"firebase": "^9.8.2",
"vuefire": "github:michi88/vuefire#firebase9-compat"

You then need to add this postinstall is in your scripts sections to build the package after npm run install finishes:

"build:vuefire": "npm --prefix node_modules/vuefire run build",
"postinstall": "npm run build:vuefire"

Then you need to change your imports like this:

import { firestoreAction, vuexfireMutations } from "vuefire/packages/vuexfire";

This is the only way I could make it work. If anyone has better ideas, let me know.

This would not be needed if we build and publish a compat version like this. Let me know if that is a route we could take @posva.

@Hibrix-net
Copy link

Made a branch here that can be installed like this in package.json:

"firebase": "^9.8.2",
"vuefire": "github:michi88/vuefire#firebase9-compat"

You then need to add this postinstall is in your scripts sections to build the package after npm run install finishes:

"build:vuefire": "npm --prefix node_modules/vuefire run build",
"postinstall": "npm run build:vuefire"

Then you need to change your imports like this:

import { firestoreAction, vuexfireMutations } from "vuefire/packages/vuexfire";

This is the only way I could make it work. If anyone has better ideas, let me know.

This would not be needed if we build and publish a compat version like this. Let me know if that is a route we could take @posva.

That could work for Firebase, but the Vuefire/Vuexfire compatibility with Vue 3/Vuex 4 is still needed anyway and in my modest opinion it is more important and urgent. There will be somebody that can take over this fantastic plugin and keep working on it sometimes.

@michi88
Copy link

michi88 commented Jun 13, 2022

@posva could you please let me know if you'd consider releasing a firebase 9 compat version? Because if not, I'll consider releasing separate npm packages as I need this in several projects and installing from source is error prone due to dev environments.

Thanks!

@michi88
Copy link

michi88 commented Aug 30, 2022

@posva could you please let me know, as else I'll release and publish a fork that is compatible with firebase 9. Thanks!

@posva
Copy link
Member

posva commented Sep 17, 2022

@michi88 I will start working on a new version of Vuefire, compatible with Firebase 9 and more, in the following weeks.

@dosstx
Copy link

dosstx commented Sep 17, 2022

Is there a reason to use the VueUse firebase package over this one?

@sceee
Copy link
Author

sceee commented Sep 17, 2022

I would also be interested in knowing what you consider the "best" way forward - whether it will be Vue 3 + Pinia + Firebase 9 (how intergrated in Pinia?) or Vue 3 + vuex + "new" vuexfire (with support for Firebase 9).
Any information is appreciated, thanks!

@liquidvisual
Copy link

Pinia would be preferred as it's the new default state manager for Vue. Vuex has been put into maintenance mode.

@posva
Copy link
Member

posva commented Oct 13, 2022

Thanks everybody for the feedback 🙏.

I'm currently migrating the library and I hope to start releasing alphas soon. You can follow the advancement at #1241

@posva posva closed this as completed Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests