The callback of the useQuery function causes batch state update in the development build

[namnguyenaurecon]

Describe the bug

When making a request and handle changing states inside the onSuccess callback of the useQuery function, the state changes are being batched in the development but not being batched in the production build, the correct one that I think should be the production build since the onSuccess callback isn't a React-based event so the state changes shouldn't be batched. That causes inconsistency between the development build and production build and led to a potential bug if the state update is working differently.

I dived deeper into the call stacks and found out that the batchNotifyFn function of the notifyManager was pointing to two different sources between the development build and production build, which might explain why the differences between 2 builds. Look at the screenshot below to see the differences.

My code snippet in case to reproduce the issue.

import { useEffect, useState } from "react";
import { useQuery } from "react-query";

const TestState = () => {
  const [count, setCount] = useState(0);
  const [count1, setCount1] = useState(0);

  useQuery<any>(
    ["getDataAsync"],
    () => getDataAsync("https://goweather.herokuapp.com/weather/Hochiminh"),
    {
      onSuccess: (data: any) => {
        handleSetCount();
      },
    }
  );

  const getDataAsync = async (apiUrl: string): Promise<any> => {
    const response = await fetch(apiUrl);
    return response.json();
  };

  useEffect(() => {
    console.log("count", count);
    console.log("count1", count1);
  }, [count, count1]);

  const handleSetCount = () => {
    setCount(count + 1);
    setCount1(count1 + 1);
  };

  return (
    <div>
      <label>{`Current count in Project is ${count}`}</label>
      <br />
    </div>
  );
};

export default TestState;

Expected behavior The onSuccess callback of the useQuery function should be working consistently between the development build and production build.

Screenshots

Development build

Production build

Desktop (please complete the following information):

• OS: Windows
• Browser: Chrome/Edge
• Version: Chrome 93.0.4577.63 / Edge 93.0.961.47

[TkDodo]

not sure this is something that react-query is really doing per se on purpose / on it's own. According to the code comment, we're just using what react gives us out of the box for batching, with an option to override it:

https://github.com/tannerlinsley/react-query/blob/ac342e237ae9fab6759f2d7be616662da6c16225/src/core/notifyManager.ts#L85-L91

we set it here:

https://github.com/tannerlinsley/react-query/blob/ac342e237ae9fab6759f2d7be616662da6c16225/src/react/setBatchUpdatesFn.ts#L1-L4

and unstable_batchedUpdates comes from react-dom:

https://github.com/tannerlinsley/react-query/blob/126773133b2f3086f68b361fedf0f0e90f29e963/src/react/reactBatchedUpdates.ts#L1-L2

so maybe React is just doing something different in development / production mode?

[namnguyenaurecon]

Thanks for your response @TkDodo , not really sure about the differences, however when I tried to put a breakpoint in setBatchNotifyFunction function, there was no trigger in the production build.

https://github.com/tannerlinsley/react-query/blob/ac342e237ae9fab6759f2d7be616662da6c16225/src/core/notifyManager.ts#L90

I went more further down to where the function gets called (not sure it's correct), the interesting thing is the import wasn't bundled to the final optimised production build. So I guess that's why the function didn't get called.

https://github.com/tannerlinsley/react-query/blob/b88cda4105d4723132f53ecbd2f5b90a96ee2228/src/react/index.ts#L2

That would be something I have to figure out myself, however, really appreciate if you could put some inputs here.

[TkDodo]

@namnguyenaurecon which tool are you using for making your production build, and which version of React are you using?

Maybe you could create a small CRA (Create React App) setup to see if it works there? Because if it doesn't, maybe there is an issue in react-query after all ...

[namnguyenaurecon]

@TkDodo , I was using CRA to create a simple app and testing on that. Also, I was thinking that was an issue regarding Webpack Tree Shaking but that wasn't, you guys already added setBatchUpdatesFn as a side effect so that wouldn't be the case that causes the issue.

[TkDodo]

Can you share the CRA so that I can take a look please?

[richardaum]

I really don't think it's related to something not being imported on production. Still trying to figure out what it could be.

[namnguyenaurecon]

@TkDodo , sorry for being late, quite busy these days, please see my repo for the app that I was using to test https://github.com/namnguyenworkspsace/react-query-test

[richardaum]

We could safely discard React being the root problem:

  // useQuery<any>(
  //   ["getDataAsync"],
  //   () => getDataAsync("https://goweather.herokuapp.com/weather/Hochiminh"),
  //   {
  //     onSuccess: (data: any) => {
  //       handleSetCount();
  //     },
  //   }
  // );

  useEffect(() => {
    const batchedSetCount = unstable_batchedUpdates(() => {
      handleSetCount();
    });
    setTimeout(() => batchedSetCount(), 2000);
  }, []);

I replaced useQuery for an useEffect + setTimeout. The results were the same batching on both dev and production modes. That means react-query is not properly batching onSuccess's calls on production mode. Still trying to understand why.

[richardaum]

Second shot:

I was thinking that was an issue regarding Webpack Tree Shaking but that wasn't, you guys already added setBatchUpdatesFn as a side effect so that wouldn't be the case that causes the issue.

I was reading about Tree Shaking here (https://webpack.js.org/guides/tree-shaking/) and adding a file to sideEffects array as react-query does with setBatchUpdatesFn and setLogger actually sign to webpack that it can prune them:

The new webpack 4 release expands on this capability with a way to provide hints to the compiler via the "sideEffects" package.json property to denote which files in your project are "pure" and therefore safe to prune if unused.

That is precisely what is going on because if I tried to search for setBatchUpdatesFn(...) on production bundle I couldn't find it (disabling minification helped to see it is not there). But if I tried to search for it on development bundle it was there.

@TkDodo There is two possible solutions here:

1. (Quick) Add a recommendation to include the following code on the application to enable batching updates:

   import { useQuery, notifyManager } from "react-query";
   import { unstable_batchedUpdates } from "react-dom";
   notifyManager.setBatchNotifyFunction(unstable_batchedUpdates);

2. (Slow) Remove setBatchUpdatesFn from sideEffects array on package.json and release a new package version. I've already tried to remove it from there and generate a new version locally and it worked.

[TkDodo]

That’s not how I read the webpack docs. sideEffects: false means we have no side effects and that everything can be shaken away. By providing an array, we tell the bundler which files have sideeffects and thus cannot be tree shaken. Is that not right?

[namnguyenaurecon]

That’s not how I read the webpack docs. sideEffects: false means we have no side effects and that everything can be shaken away. By providing an array, we tell the bundler which files have sideeffects and thus cannot be tree shaken. Is that not right?

That's what I thought the tree shaking would work that way, however, it seems to be working the opposite way, I've been searching and doing somethings to prove it's wrong.

[richardaum]

Yeah... I've tried the second option again (removing setBatchUpdatesFn from sideEffects) - and it didn't work. Maybe my last build was still considering the first option. And I misread the webpack docs - sorry for that. You guys are right, that option should mark a file as side effect and should prevent from being removed.

But still, the setBatchUpdatesFn file isn't being considered when running on production. I don't know why. Using setBatchNotifyFunction looks like a temporary working solution but does not explain why it its necessary.

[TkDodo]

Can we try a different bundler like rollup to see if the issue persists there?

[richardaum]

Do you mean a bundler for the application? I'm using Vite (which is an implementation over Rollup) as bundler and the issue persists.

[richardaum]

Another interesting thing: node_modules/react-query/lib/react/index.js which should call setBatchUpdatesFn actually isn't being called. I added a console.log on it and it isn't being executed.

In other hard, I've tried to do same on node_modules/react-query/es/react/useQuery.js which is directly imported by the app. The console.log properly worked.

So the issue could be related to how the application is bundling its dependency into a single file (vendor or bundle). I'm starting to think we should export side effects in a way we could use them directly on application - that could be because the builder may be stripping out side effects on node_modules.

[richardaum]

Third shot:

  "sideEffects": [
    "es/index.js",
    "es/react/index.js",
    "es/react/setBatchUpdatesFn.js",
    "es/react/setLogger.js",
    "lib/index.js",
    "lib/react/index.js",
    "lib/react/setBatchUpdatesFn.js",
    "lib/react/setLogger.js"
  ],

If I change sideEffects on react-query's package.json to the above snippet, it works.

~Maybe this is not the best way, I'm looking for the pure annotation to check if it works, becuase doing this way may hurt the tree shaking.~ Pure annotation does not work as I thought.

[TkDodo]

It seems like “fixing” sideEffects is the right way to go, however, adding index.js might make the whole app non-treeshakeable? Or is this actually correct given that index.js is the file that has the side-effect by importing logger.js 🤔

Can you check how this change affects bundle size?

[richardaum]

Bundle sizes:

Not adding index.js files to sideEffects dist/assets/vendor.b6c55d52.js 437159 bytes (13737 lines unminified)

Adding index.js files to sideEffects dist/assets/vendor.776ca1ef.js 437465 bytes (13746 lines unminified)

Difference between original and proposal builds:

index 873693b..333eb47 100644
--- a/dist/assets/vendor.b6c55d52.js
+++ b/dist/assets/vendor.776ca1ef.js
@@ -7584,16 +7584,16 @@ var Recoil_RecoilValueInterface = {
   invalidateDownstreams_FOR_TESTING: invalidateDownstreams
 };
 const {
-  unstable_batchedUpdates
+  unstable_batchedUpdates: unstable_batchedUpdates$1
 } = ReactDOM;
 var ReactBatchedUpdates = {
-  unstable_batchedUpdates
+  unstable_batchedUpdates: unstable_batchedUpdates$1
 };
 const {
-  unstable_batchedUpdates: unstable_batchedUpdates$1
+  unstable_batchedUpdates: unstable_batchedUpdates$1$1
 } = ReactBatchedUpdates;
 var Recoil_ReactBatchedUpdates = {
-  unstable_batchedUpdates: unstable_batchedUpdates$1
+  unstable_batchedUpdates: unstable_batchedUpdates$1$1
 };
 const {
   batchStart: batchStart$1
@@ -12099,13 +12099,16 @@ var NotifyManager = /* @__PURE__ */ function() {
   return NotifyManager2;
 }();
 var notifyManager = new NotifyManager();
-var logger = console || {
+var logger$1 = console || {
   error: noop,
   warn: noop,
   log: noop
 };
 function getLogger() {
-  return logger;
+  return logger$1;
+}
+function setLogger(newLogger) {
+  logger$1 = newLogger;
 }
 var Query = /* @__PURE__ */ function() {
   function Query2(config) {
@@ -13600,6 +13603,12 @@ function shouldFetchOptionally(query, prevQuery, options, prevOptions) {
 function isStale(query, options) {
   return query.isStaleByTime(options.staleTime);
 }
+var unstable_batchedUpdates = ReactDOM.unstable_batchedUpdates;
+notifyManager.setBatchNotifyFunction(unstable_batchedUpdates);
+var logger = console;
+if (logger) {
+  setLogger(logger);
+}
 var defaultContext = /* @__PURE__ */ React.createContext(void 0);
 var QueryClientSharingContext = /* @__PURE__ */ React.createContext(false);
 function getQueryClientContext(contextSharing) {

[richardaum]

It looks like the only difference is the side effects addition to the bundle and some variable renaming. What do you think @TkDodo?

[namnguyenaurecon]

@TkDodo @richardaum , I've read through the tree shaking clarification once again, that looks like adding index.js as a side effect will address the issue since that meets the expectation and that doesn't make the whole app side effect free. still makes the whole app tree-shakable.

https://webpack.js.org/guides/tree-shaking/#clarifying-tree-shaking-and-sideeffects

[TkDodo]

I think this looks good and the bundle sizes changes are expected and bring back the missing function. Can you create a PR please? Thank you all for your awesome work on this 🙏

[richardaum]

I think this looks good and the bundle sizes changes are expected and bring back the missing function. Can you create a PR please? Thank you all for your awesome work on this 🙏

https://github.com/tannerlinsley/react-query/pull/2703

[richardaum]

@namnguyenaurecon a new version was released with the fix. could you try it and check if your application is fixed?

[namnguyenaurecon]

@namnguyenaurecon a new version was released with the fix. could you try it and check if your application is fixed?

Just checked the new version, it works like a charm !!!

Thanks guys for the awesome work @TkDodo @richardaum .