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

Migrate to @tanstack/react-query (react-query v4) #61

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

kaueDM
Copy link

@kaueDM kaueDM commented Aug 24, 2022

Overview

This PR migrates react-query from v3 to v4 and:

  • Fixes external dependencies for rollup builds
  • Add support to local tarballs with npm pack (npx lerna run pack)

Tests

I couldn't run the tests, got some weird errors

Caveats

I couldn't build using the current UseFirestoreHookOptions type, as a workaround I merged both GetSnapshotOptions and SnapshotListenOptions:

- export type UseFirestoreHookOptions =
-   | ({
-       subscribe: true;
-     } & SnapshotListenOptions)
-   | ({
-       subscribe?: false;
-     } & GetSnapshotOptions);
+ export type UseFirestoreHookOptions = {
+   subscribe?: boolean;
+ } & GetSnapshotOptions &
+   SnapshotListenOptions;

If someone finds a better way to handle this type, suggestions are welcome.

Related issues

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2022

CLA assistant check
All committers have signed the CLA.

@cabljac
Copy link
Contributor

cabljac commented Aug 24, 2022

Great work! Thanks for contributing!

I will review and publish a dev release so people can try it out.

As for the tests, I'm pretty sure most of our subscription tests need rewriting for v4, as we currently rely on the onSuccess method being fired, which no longer happens.

@kaueDM
Copy link
Author

kaueDM commented Aug 24, 2022

Great work! Thanks for contributing!

I will review and publish a dev release so people can try it out.

As for the tests, I'm pretty sure most of our subscription tests need rewriting for v4, as we currently rely on the onSuccess method being fired, which no longer happens.

I can take a look on the broken tests too ASAP, I didn't saw #59 .

My issue is on starting the test suite locally, I always get an error related with --runInBand. Tried to google it but couldn't find anything useful. Any idea why this is happening?

image

@cabljac
Copy link
Contributor

cabljac commented Aug 24, 2022

hmm haven't seen that before. I don't get that when i run them. Perhaps it's trying to use a different version of jest somehow? not sure.

@Shahzad6077
Copy link

Hey @kaueDM, are you able to fix the test error?

@jasuno
Copy link

jasuno commented Oct 13, 2022

Is this getting released?

@adamgajzlerowicz
Copy link

Please release! ❤️

@kaueDM
Copy link
Author

kaueDM commented Nov 8, 2022

I'm currently out of time to take a look on the tests. If someone could help here, it'll be appreciated.

@adamgajzlerowicz
Copy link

a third of tests is failing 😁

@kaueDM
Copy link
Author

kaueDM commented Nov 8, 2022

a third of tests is failing 😁

Expected. I couldn't run the tests, so I didn't changed anything.

@kaueDM kaueDM closed this Nov 8, 2022
@kaueDM kaueDM reopened this Nov 8, 2022
@kaueDM
Copy link
Author

kaueDM commented Nov 8, 2022

Opps, missclick on the close with comment button

@adamgajzlerowicz
Copy link

adamgajzlerowicz commented Nov 8, 2022

Tests are working for me.
This is what I came up with:

    test("it subscribes and unsubscribes to data events", async () => {
      const hookId = genId();
      const id = genId();
      const col = collection(firestore, "tests", id, id);
      const ref = query(col, orderBy("order", "asc"));


      const { unmount, result } = renderHook(
        () => {
          const { data } = useFirestoreQuery(
            [hookId],
            ref,
            {
              subscribe: true,
            },
          );
          return data;
        },
        {
          wrapper,
        }
      );


      await act(async () => {
        await addDoc(col, { foo: "bar", order: 0 });
        await addDoc(col, { foo: "baz", order: 1 });
      });


      // Trigger an  unmount - this should unsubscribe the listener.
      unmount();

      // Trigger an update to ensure the mock wasn't called again.
      await act(async () => {
        await addDoc(col, { foo: "..." });
      });


      expect(result.current.size).toEqual(2);

      expect(result.current.docs[0].data().foo).toEqual("bar");
      expect(result.current.docs[0].data().order).toEqual(0);
      expect(result.current.docs[1].data().foo).toEqual("baz");
      expect(result.current.docs[1].data().order).toEqual(1);
    });

It seems to bring expected result.

EDIT: Just to explain - I refactored one of the tests, that expected calls to be triggered.
Now, instead of amount of calls, I let the firestore triggers to run, and once they are finished, I expect the resulting data length to equal the length of the data that was included during the subscription.

No useEffect is needed. The tests seem simpler also.

@adamgajzlerowicz
Copy link

@kaueDM regarding your tests failing, seems that your jest is not recognising the flag. Perhaps you have old jest installed globally and it tries to use it instead of the local one? 🤔

@adamgajzlerowicz
Copy link

If you like I can attempt to fix all the tests. Will you accept pr then?

@izakfilmalter
Copy link

Is there any hope of this getting merged?

@chrisk8er
Copy link

when it releases?

@MartinJohannesNilsen
Copy link

MartinJohannesNilsen commented Dec 6, 2022

Would love to see the migration to v4 released at npm

@sa-bangash
Copy link

Any update?

@tonnoz
Copy link

tonnoz commented Jan 12, 2023

any news about this one?
myfile
Jokes apart, would be awesome to have this one merged.
Thanks for the great work guys ❤

@birosrichard
Copy link

Is this gonna be released someday?

@chrisk8er
Copy link

I'm using react query 4, and I need this library for my new project... please @cabljac 🙂🙏🏻

@pawKer
Copy link

pawKer commented Feb 10, 2023

+1 for this being merged in 😄 🙏

@tonnoz
Copy link

tonnoz commented Apr 18, 2023

any news about this?

@kaumac
Copy link

kaumac commented May 18, 2023

I just started looking into react query firebase, is this getting merged at all?

@tonnoz
Copy link

tonnoz commented May 26, 2023

loosing hope for this merge. Might go back to ReactFire I think :(

@weslleyaraujo
Copy link

Hello @Salakar 👋, I came across react-query-firebase while exploring options for a new project. I noticed this PR and wanted to ask if Invertase has any plans to continue maintaining the library in the future.

Do you have any advice for someone interested in using react-query-firebase? Thank you 🙏

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.