TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
40.3k stars 2.7k forks source link

ESM support #3882

Closed TkDodo closed 1 year ago

TkDodo commented 1 year ago

we had it in v4 beta, but it went away with the rebranding.

prior art:

there were some useSyncExternalStore issues reported after that, which prompted another fix:

TkDodo commented 1 year ago

@sachinraja FYI

TkDodo commented 1 year ago

let's try to keep discussion about ESM support here :)

I tried out 4.0.11-beta.0 and it works fine in our simple example, but that one uses vite:

There have been reports on discord that this version "breaks the app", but we'd need more info and try out more bundlers I guess.

cc @DamianOsipiuk

sachinraja commented 1 year ago

@TkDodo Yeah if they share reproductions or at least error messages, I can look into it.

TkDodo commented 1 year ago

@sachinraja I can confirm that 4.0.11-beta.0 doesn't work with react-scripts v4. Here is an (older) repo of mine that uses v4 of react-scripts, and I've merely switched from 4.1.0 where it works (but doesn't render devtools at all as per the issue) to the beta:

what I'm seeing when I run it is this:

Screenshot 2022-08-12 at 20 06 57

I can also confirm that removing the rendering of the <ReactQueryDevtools initialIsOpen /> makes the error go away.

TkDodo commented 1 year ago

Another thing I found out with @DamianOsipiuk is that the size of the beta is 5x times too large:

https://bundlephobia.com/package/@tanstack/react-query@4.0.11-beta.0

I think @DamianOsipiuk mentioned that react-dom is included because it wasn't specified as external. Just wanted to note all known issues here :)

sachinraja commented 1 year ago

I've seen that Provider error before, it has to do with bundlers picking the cjs format for one package and the same format for the other package.

TkDodo commented 1 year ago

@DamianOsipiuk I tried the latest beta (4.0.11-beta.4) with react-scripts v4 and it now fails with:

Screenshot 2022-08-18 at 14 06 25

Failed to compile.

./node_modules/@tanstack/react-query-devtools/build/lib/index.mjs
Can't import the named export 'Fragment' from non EcmaScript module (only default export is available)
DamianOsipiuk commented 1 year ago

Hmm it seems that it now complains about esm + cjs interop. At least it's picking up correct entry right now. Will take a look at that.

DamianOsipiuk commented 1 year ago

So far when i have played with it i was unable to make it work for webpack 4 while maintaining support for other bundlers.

There is additional config needed for webpack 4 to make it work, which will be a pain for react-scripts users due to webpack config being inaccessible without third party tools.

I have one more idea which i will test when i will have some time, via exports.node.

All other ideas to solve this problem appreciated.

TkDodo commented 1 year ago

can I ask all interested parties to please try out the latest and greatest beta version:

https://github.com/TanStack/query/releases/tag/v4.3.0-beta.3

on my create-react-app v4, I see no problems now:

TkDodo commented 1 year ago

One thing that doesn't work is the lazy import of the devtools in production. looking at our exports, there should be a .production entry point:

https://github.com/TanStack/query/blob/2452ec1d81a20bd8990ee6f3dc8a04e4ad560b31/packages/react-query-devtools/package.json#L15-L34

However, if I'm trying:

const ReactQueryDevtoolsProduction = React.lazy(() =>
    import('@tanstack/react-query-devtools/production').then(d => ({
        default: d.ReactQueryDevtools,
    }))
)

I'm getting the error:

TS2307: Cannot find module '@tanstack/react-query-devtools/production' or its corresponding type declarations.
     7 |
     8 | const ReactQueryDevtoolsProduction = React.lazy(() =>
  >  9 |     import('@tanstack/react-query-devtools/production').then(d => ({
       |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    10 |         default: d.ReactQueryDevtools,
    11 |     }))
    12 | )

I tried this with create-react-app v4 and v5

TkDodo commented 1 year ago

I also tried out our nextJs example and it works fine with the beta:

https://codesandbox.io/s/recursing-wind-vo3htt

TkDodo commented 1 year ago

okay one more tiny thing: it seems that direct dependencies of the devtools are now included in the production bundle:

https://unpkg.com/@tanstack/react-query-devtools@4.3.0-beta.3/build/umd/index.production.js

I can see useSyncExternalStore and match-sorter-utils being bundled.

this is also visible in bundlephobia: https://bundlephobia.com/package/@tanstack/react-query-devtools@4.3.0-beta.3

Screenshot 2022-08-23 at 10 22 48

4.2.1 has 317B while 4.3.0-beta.3 has 8.2kB

DamianOsipiuk commented 1 year ago

@TkDodo I would argue that devtools should actually bundle those. From my perspective for UMD you should only include https://unpkg.com/@tanstack/react-query@4.3.0-beta.3/build/umd/index.production.js and https://unpkg.com/@tanstack/react-query-devtools@4.3.0-beta.3/build/umd/index.production.js

Without the need of importing additional intermediate dependencies. This is also the case for react-query which contains query-core bundled, so users do not have to think about importing it.

What do you think?


The other issue is that esm and cjs builds should not contain them - this needs to be fixed 😉

TkDodo commented 1 year ago

@DamianOsipiuk I generally agree with that, however, the production devtools should effectively be "empty". The Devtools and DevtoolsPanel is just a function that returns null - no dependency is needed. Especially match-sorter is easily visible, and I've also done a production build of a testing repo, where I can see that match-sorter is also included, even though it is not used :)

The other issue is that esm and cjs builds should not contain them - this needs to be fixed 😉

Ha, okay :)

DamianOsipiuk commented 1 year ago

Oh, right. They need to be bundled in dev only. :+1:

TkDodo commented 1 year ago

any idea what to do about the production devtools lazy import?

DamianOsipiuk commented 1 year ago

Not yet, but i will look into it.

TkDodo commented 1 year ago

testing with 4.3.0-beta.4:

I can see the separate bundle being created, and it also gets loaded in the browser network tab when I do toggleDevtools(), however, it renders nothing. If I start the app in dev mode, it works. This is really weird because the bundle seems correct just from looking at it 🤔 .

looked into this for a bit now, it seems to be something with our useIsMounted hook:

https://github.com/TanStack/query/blob/0fda41a56c7fd485650415626b2a0f1d046a4957/packages/react-query-devtools/src/utils.ts#L103-L115

the ref is never set to true, therefore, we render nothing. Don't know why yet...

DamianOsipiuk commented 1 year ago

however, that import path is not great. Could we somehow alias that to '@tanstack/react-query-devtools/production' ?

I can reintroduce that export, but it will only work with bundlers that fully supports exports, like vite. react-scripts would have to import the ugly one Unfortunately the issue with @tanstack/react-query-devtools/production is typescript bug that i have mentioned previously. So typescript will yell at you that it does not have type declarations, even though vite will compile successfully.

rendering the production devtools doesn't actually work in a production build. Here is the code:

I'm not sure about this particular piece of code but it worked for me with the following with react-scripts 4:

import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
// import { ReactQueryDevtools } from "@tanstack/react-query-devtools";
import React from "react";
import { Example } from "./Example";

const queryClient = new QueryClient();

const ReactQueryDevtools = React.lazy(async () => {
  const devtools = await import(
    "@tanstack/react-query-devtools/build/lib/index.prod.js"
  );

  return {
    default: devtools.ReactQueryDevtools,
  };
});

function App() {
  return (
    <QueryClientProvider client={queryClient}>
      <Example />
      <ReactQueryDevtools initialIsOpen />
    </QueryClientProvider>
  );
}

export default App;
TkDodo commented 1 year ago

I can reintroduce that export, but it will only work with bundlers that fully supports exports, like vite. react-scripts would have to import the ugly one

if that's fine with you, i think its okay.

I'm not sure about this particular piece of code but it worked for me with the following with react-scripts 4:

you are right! it works fine with react-scripts 4, but not with react-scripts 5. I up/downgraded back and forth for testing, and when I wrote the above summary, I missed that.

DamianOsipiuk commented 1 year ago

@TkDodo additional import - https://github.com/TanStack/query/pull/4090 ~not sure what could be the problem but you core works for me both for react scripts 4 and 5~

Edit. It does not work with React.lazy


So it seems, when you change

React[isServer ? 'useEffect' : 'useLayoutEffect'](() => { 

to

React['useEffect'](() => { 

or

React['useLayoutEffect'](() => { 

It starts working again...

TkDodo commented 1 year ago

@DamianOsipiuk that's an interesting find! we need the isServer check because useLayoutEffect doesn't work on the server. However, I think we could simply always useEffect here? Or maybe it also works if we change isServer to be a function, because then the result cannot be optimized away:

- export const isServer = typeof window === 'undefined'
+ export const isServer = () => typeof window === 'undefined'

- React[isServer ? 'useEffect' : 'useLayoutEffect'](() => { 
+ React[isServer() ? 'useEffect' : 'useLayoutEffect'](() => {

?

TkDodo commented 1 year ago

@DamianOsipiuk I've switched to only using useEffect and removed the isServer check completely:

I'll quickly re-test this with the latest beta. This was the last issue, right? If that works, we would be ready to merge?

TkDodo commented 1 year ago

@DamianOsipiuk toggling devtools now works in react-scripts v5. The typescript import issue is still there, but I think for now, I'll just add documentation that you can import with the long path.

DamianOsipiuk commented 1 year ago

@TkDodo Typescript issue will go away if you change the moduleResolution to NodeNext. We could potentially add that to docs.

And yes, i think that was the last issue.

TkDodo commented 1 year ago

I added this paragraph, is that about right?

https://github.com/TanStack/query/commit/5e145fcced65b0bbba3984da93e780357697e47f

DamianOsipiuk commented 1 year ago

Yup, although i think that TS 4.7 is required.

TkDodo commented 1 year ago

I got 4.5 from their docs:

DamianOsipiuk commented 1 year ago

I was more thinking about this https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing 😉

TkDodo commented 1 year ago

okay, I updated it: 40c34910

igortas commented 12 months ago

@TkDodo Version 5.0.0-beta-alpha.56 complains for lazy loding dev tools for production, same goes for lower version, at least I've tried 35 and it's the same... Tried adding index.d.ts file with declare module '@tanstack/react-query-devtools/build/lib/index.prod.js', not helping too...

Module not found: Error: Package path ./build/lib/index.prod.js is not exported from package /home/igor/projects/ToolbarApp/node_modules/@tanstack/react-query-devtools (see exports field in /home/igor/projects/ToolbarApp/node_modules/@tanstack/react-query-devtools/package.json)

TkDodo commented 12 months ago

That's more something for @ardeora