Compatibility with Firebase Modular JS SDK (v9)

sceee commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

Do you think this would make sense?

Yes, feel free to do it

sceee commented 3 years ago

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 commented 3 years ago

@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

First install firebase-tools globally if you don't yet have them: npm i -g firebase-tools
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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Thanks a lot @sceee !

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

dosstx commented 3 years ago

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 commented 3 years ago

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

sceee commented 3 years ago

@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 commented 3 years ago

I think it can be ignored

bangdragon commented 3 years ago

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

gabrielnvian commented 2 years ago

What's the progress on this?

sceee commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

tmaly1980 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

posva commented 1 year ago

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

dosstx commented 1 year ago

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

sceee commented 1 year ago

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 commented 1 year ago

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

posva commented 1 year ago

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 https://github.com/vuejs/vuefire/issues/1241

vuejs / vuefire