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

React Native: Queries not executing on v4 #3582

Closed
sandergo90 opened this issue May 3, 2022 · 34 comments
Closed

React Native: Queries not executing on v4 #3582

sandergo90 opened this issue May 3, 2022 · 34 comments
Labels

Comments

@sandergo90
Copy link
Contributor

Describe the bug

Hi,

I'm trying out 4.0.0-beta.7 to implement offline support in our application.

Everything is upgraded including all the breaking changes, but my queries are not being executed anymore.
To be sure it's not a problem inside my application, I've created a fresh expo app with just the App.tsx file.

When opening the app, my queries remain in a loading state. If I use the refetch of the useQuery, and run this in a useEffect, I'm able to execute the query.

Your minimal, reproducible example

Couldn't get react-query to work in expo snack

Steps to reproduce

  1. Create a react-native application with the following package.json file
{
  "name": "reactqueryv4",
  "version": "1.0.0",
  "main": "node_modules/expo/AppEntry.js",
  "scripts": {
    "start": "expo start",
    "android": "expo start --android",
    "ios": "expo start --ios",
    "web": "expo start --web",
    "eject": "expo eject"
  },
  "dependencies": {
    "expo": "~44.0.0",
    "expo-status-bar": "~1.2.0",
    "react": "17.0.1",
    "react-dom": "17.0.1",
    "react-native": "0.64.3",
    "react-native-web": "0.17.1",
    "react-query": "4.0.0-beta.7"
  },
  "devDependencies": {
    "@babel/core": "^7.12.9",
    "@types/react": "~17.0.21",
    "@types/react-native": "~0.64.12",
    "typescript": "~4.3.5"
  },
  "private": true
}
  1. Create a query with a simple async function
import { Text, View } from "react-native";
import { QueryClient, QueryClientProvider, useQuery } from "react-query";

export default function App() {
  const queryClient = new QueryClient();

  return (
    <QueryClientProvider client={queryClient}>
      <Todos />
    </QueryClientProvider>
  );
}

function Todos() {
  // Queries
  const query = useQuery(["todos"], async () => {
    console.log("this should log but doesn't");

    return Promise.resolve("ok");
  });

  console.log(query.data);

  return (
    <View>
      <Text>lol</Text>
    </View>
  );
}
  1. Open your application and you will see undefined query data, a status loading and no console.log.

Expected behavior

I expect this query to be executed. If I change my package.json to the latest v3 version, my query get's executed as it should.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • iOS, Android

react-query version

4.0.0-beta.7

TypeScript version

No response

Additional context

No response

@teaguestockwell
Copy link

teaguestockwell commented May 3, 2022

I am also having the same issue using 4.0.0.beta.7 where querys are stuck in a loading state. It looks like they never calling the query function.

I created a repo that reproduces this issue on react native as well as an equivalent code sandbox for web that does not have this issue
image

@hirbod
Copy link
Contributor

hirbod commented May 4, 2022

I can exactly confirm this issue, which I already tried to investigate with @TkDodo . The last working version of RQ was v4 alpha 24. But you guys are reporting iOS as well, but for me only android was breaking while iOS works just fine

@sandergo90
Copy link
Contributor Author

Well, at first my iOS version was also still working. After a clean app install this issue was also happening on iOS. And if you create a fresh react-native project, you will see it happening on both iOS and Android.

@TkDodo
Copy link
Collaborator

TkDodo commented May 5, 2022

So what we changed was to switch to useSyncExternalStore for our subscriptions. If the query never fires (= there is no request that reaches the backend), that likely means that the subscription wasn't created for some reason. It would be good to inspect the number of observers for each query. If they are zero, that would confirm my theory. Maybe you can do that via the devtools, or via simply inspecting / logging from the queryClient.

Also, can someone try with RN 0.69 RC, which supports react18. With react17, we fall back to the uSES shim, so behavior could be different:
reactwg/react-native-releases#21

@hirbod
Copy link
Contributor

hirbod commented May 5, 2022

RN 0.69 won't be an option for Expo users for at least 3-6 months. This is usually the period updates will be shipped, and a really big user base does stick with Expo. We really need to figure this out. I will check the observers with flipper and report back.

@TkDodo
Copy link
Collaborator

TkDodo commented May 5, 2022

@hirbod you mentioned that things were working fine for you on RN 0.64.3, and the issue first showed up when you updated to RN 0.68.1

OP says things are also faulty with 0.64. can we try to narrow down if there is a version of RN that works with the current beta?

@hirbod
Copy link
Contributor

hirbod commented May 5, 2022

That is true, for me the issues showed up when I upgraded. OP also said that iOS was working fine after upgrade but stopped working after clean install (which I didn't for iOS but for Android).

Looks like that the beta isn't working at all with the current RN versions. (0.69 not tested, since I am also an Expo user)

@hirbod
Copy link
Contributor

hirbod commented May 5, 2022

@TkDodo I've upgraded and you're right, no observers.

Bildschirmfoto 2022-05-05 um 21 13 27

After downgrade to alpha.24
Bildschirmfoto 2022-05-05 um 21 22 39

@TkDodo
Copy link
Collaborator

TkDodo commented May 6, 2022

@hirbod the observers are subscribed when useSyncExternalStore picks up a change. The subscription also triggers the first fetch. It should happen here:
https://github.com/tannerlinsley/react-query/blob/fdbc002006d7d8c3e236da71cb0c23cf394c1835/src/reactjs/useBaseQuery.ts#L86

Could you check if it's called at all, or if the isRestoring check has something to do with it?

It could be a bug in the uSES shim if the function is not invoked at all. We're also not on the latest uSES version, maybe we should try upgrading

@hirbod
Copy link
Contributor

hirbod commented May 6, 2022

@TkDodo looks like its never calling it. I was not able to reach neither "undefined" or the else statement.
am I doing something? gets triggered though. I tried adding some console statements to the transpiled file.

Bildschirmfoto 2022-05-06 um 07 56 53

isRestoring is always "false"

@hirbod
Copy link
Contributor

hirbod commented May 6, 2022

I forced use-sync-external-store v.1.1.0 via yarn resolutions, but that does not fix the issue either.

yarn why v1.22.17
[1/4] 🤔  Why do we have the module "use-sync-external-store"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#zustand" depends on it
   - Hoisted from "_project_#zustand#use-sync-external-store"
   - Hoisted from "_project_#app#use-sync-external-store"
   - Hoisted from "_project_#app#react-query#use-sync-external-store"
info Disk size without dependencies: "104KB"
info Disk size with unique dependencies: "104KB"
info Disk size with transitive dependencies: "104KB"
info Number of shared dependencies: 0
✨  Done in 0.71s.

@TkDodo TkDodo changed the title Queries not executing on v4 React Native: Queries not executing on v4 May 7, 2022
@TkDodo TkDodo added bug Something isn't working v4 labels May 7, 2022
@TkDodo
Copy link
Collaborator

TkDodo commented May 7, 2022

Can you try out if other hooks that subscribe via uSES work? For example: one button that does queryClient.fetchQuery, and somewhere else a component that does useIsFetching(); the count should change when you click the button. Make sure the useIsFetching component is in a different part if the component tree that would not rerender top-down when the button is clicked.
If that works, it points towards an issue in useBaseQuery only. If not, it's likely uSES related. The useIsFetching implementation is pretty minimal:
https://github.com/tannerlinsley/react-query/blob/fdbc002006d7d8c3e236da71cb0c23cf394c1835/src/reactjs/useIsFetching.ts#L26-L34

@satya164
Copy link
Contributor

satya164 commented May 8, 2022

This line in useBaseQuery imports useSyncExternalStore like this:

import { useSyncExternalStore } from "use-sync-external-store/shim/index.js"

If you change it to following, it works:

import { useSyncExternalStore } from "use-sync-external-store/shim"

The shim folder does include a different file for React Native (index.native.js). If we look at contents in the repo, those files are identical, but they differ in the published package.

The code of index.js in the published package:

'use strict';

if (process.env.NODE_ENV === 'production') {
  module.exports = require('../cjs/use-sync-external-store-shim.production.min.js');
} else {
  module.exports = require('../cjs/use-sync-external-store-shim.development.js');
}

The code of index.native.js in published package:

'use strict';

if (process.env.NODE_ENV === 'production') {
  module.exports = require('../cjs/use-sync-external-store-shim.native.production.min.js');
} else {
  module.exports = require('../cjs/use-sync-external-store-shim.native.development.js');
}

So it doesn't work because the code isn't importing the React Native specific implementation.

I added a log here and it doesn't get called when using use-sync-external-store/shim/index.js but gets called with use-sync-external-store/shim. Though I tried the same code inside App.js but couldn't reproduce the issue. I guess metro is importing code differently inside node_modules? But it's pretty confusing why it would do that.

Here is a video showing that this change works:

Kapture.2022-05-09.at.00.29.14.mp4

I see that this change was made in #3561 for ESM, so I guess the fix would be for React to add a package.json inside the shim folder so that it can be imported normally, or create 2 new files that have imports for React Native and rest.

My test code:

expo init testapp
cd testapp
yarn add react-query@4.0.0-beta.7

And paste the following in App.js:

import { Text, View } from 'react-native';
import { QueryClient, QueryClientProvider, useQuery } from 'react-query';

export default function App() {
  const queryClient = new QueryClient();

  return (
    <QueryClientProvider client={queryClient}>
      <Test />
    </QueryClientProvider>
  );
}

function Test() {
  // Queries
  const query = useQuery(['test'], {
    queryFn: async () => {
      console.log("this should log but doesn't");

      return Promise.resolve('ok');
    },
  });


  return (
    <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
      <Text>Status: {query.status} </Text>
      <Text>Data: {query.data} </Text>
    </View>
  );
}

@TkDodo
Copy link
Collaborator

TkDodo commented May 9, 2022

That would mean that beta.5 release should still work (pre esm bundling, but with uSES). @hirbod can you try that please?
https://github.com/tannerlinsley/react-query/releases/tag/v4.0.0-beta.5

@TkDodo
Copy link
Collaborator

TkDodo commented May 9, 2022

@sachinraja can we go back to not including index.js for the import?

@hirbod
Copy link
Contributor

hirbod commented May 9, 2022

@TkDodo beta.5 should work. If you remember, I started with beta.4, which worked perfectly. I upgraded to beta.7 with my RN 0.68 upgrade. beta.5 does import /shim and not index.js

Gonna give it a try

edit: Confirmed @TkDodo. RN 0.68.1 and beta-5 working!

@TkDodo
Copy link
Collaborator

TkDodo commented May 9, 2022

If you remember, I started with beta.4, which worked perfectly

Seems to be a miscommunication then. Further above, in this comment, you mentioned that:

The last working version of RQ was v4 alpha 24.

beta.1 introduced the uSES, but beta.7 introduced the full-path import. Seems like we have to revert that, or find a better solution to the initial issue, which was:

#3521 (comment)

@latin-1 fyi

@hirbod
Copy link
Contributor

hirbod commented May 9, 2022

That was just the assumption after you asked me to downgrade to that version. Anyway, we have found the root cause :).

@latin-1
Copy link
Contributor

latin-1 commented May 9, 2022

@TkDodo Conditional exports in package.json might help but I'm not sure. I'm not familiar with React Native.

@satya164
Copy link
Contributor

satya164 commented May 9, 2022

Conditional exports are for Node afaik and not going to work with RN.

I think the best solution is to add a package.json under use-sync-external-store/shim with react-native entry instead of being done by each consumer of the library. If I understand correctly, it will also work with ESM because of the package.json.

@latin-1
Copy link
Contributor

latin-1 commented May 9, 2022

@satya164 facebook/metro#670 Yes you are correct. Conditional exports is not yet supported.

@sachinraja
Copy link
Contributor

React is really putting us in a difficult place here. If uSES doesn't work with that import I suspect our setBatchedUpdatesFn doesn't work either.

Does someone familiar with RN bundling know if it supports or prefers the "module" field? I don't have time to test it right now.

@sachinraja
Copy link
Contributor

Ok I have an idea on how to fix this. We can do a separate build without extensions (like we were doing before) and use the "react-native" field in each module's package.json to point at those files.

@satya164
Copy link
Contributor

satya164 commented May 9, 2022

Why not fix it in use-sync-external-store/shim? If every package using it has to do workarounds for using it then it won't be ideal. It'll be more beneficial to fix it at the source so that it's compatible with ESM.

You also don't need a separate build without an extension or a package.json. If you want to fix it in React Query instead of in use-sync-external-store, then you can create 2 files useSyncExternalStore.js and useSyncExternalStore.native.js that use different imports and exports them, and then you can import ./useSyncExternalStore .

@sachinraja
Copy link
Contributor

sachinraja commented May 9, 2022

Why not fix it in use-sync-external-store/shim?

Because we have no control over that package. @latin-1 does have a PR open at facebook/react#24440 which I've left a comment in, so hopefully this gets resolved.

you can import ./useSyncExternalStore

We cannot do that. In ESM imports, you must specify the extension.

@satya164
Copy link
Contributor

satya164 commented May 9, 2022

Because we have no control over that package

Yeah but imo it'd better to see if React team doesn't want to fix this first since it's an issue in that package, not necessarily in React Query. Probably they will after your comment.

We cannot do that. In ESM imports, you must specify the extension.

As far as I see all other files are imported like that - for example in the first import is ../core in the useBaseQuery.js file linked above. Looks like the extensions are added during build time in .mjs files which would be fine since those aren't the files being used in React Native - only the .js files are being used. So this approach seems feasible to me and is also simpler.

@sachinraja
Copy link
Contributor

Ah ok I see what you're saying now. Yes that would be much simpler. I didn't realize React Native doesn't resolve the esm files.

@hirbod
Copy link
Contributor

hirbod commented May 11, 2022

I think the import should be corrected by RQ for now. Even if the issue is fixed upstream, the problem is that many major platforms and SDKs like Expo always ship a fixed version of React. Current version 17.0.2.

If we rely only on upstream now, we run the risk of error messages continuing to come in regarding this, users overlooking the fix, and RQ having to point to a specific React version.

I think we should import it the suggested way from @satya164
#3582 (comment)

@TkDodo
Copy link
Collaborator

TkDodo commented May 11, 2022

@hirbod can you make a PR with that suggestion?

@satya164
Copy link
Contributor

the problem is that many major platforms and SDKs like Expo always ship a fixed version of React. Current version 17.0.2.

The issue is in use-sync-external-store package though, which is separate from React and can be updated to latest version.

@hirbod
Copy link
Contributor

hirbod commented May 11, 2022

True, haven't thought about it. I think we can fix it for now and revert when upstream is ready.

@hirbod
Copy link
Contributor

hirbod commented May 11, 2022

@TkDodo #3601

@tannerlinsley
Copy link
Collaborator

🎉 This issue has been resolved in version 4.0.0-beta.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@HansBrende
Copy link

@sachinraja FYI, the newer react PR for this is here: facebook/react#25231

Doesn't seem to have gotten much love 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants