-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
ESM support #3882
Comments
@sachinraja FYI |
let's try to keep discussion about ESM support here :) I tried out 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. |
@TkDodo Yeah if they share reproductions or at least error messages, I can look into it. |
@sachinraja I can confirm that
what I'm seeing when I run it is this: I can also confirm that removing the rendering of the |
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 :) |
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. |
@DamianOsipiuk I tried the latest beta (4.0.11-beta.4) with
|
Hmm it seems that it now complains about |
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 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. |
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:
|
One thing that doesn't work is the lazy import of the devtools in production. looking at our query/packages/react-query-devtools/package.json Lines 15 to 34 in 2452ec1
However, if I'm trying:
I'm getting the error:
I tried this with create-react-app v4 and v5 |
I also tried out our nextJs example and it works fine with the beta: |
okay one more tiny thing: https://unpkg.com/@tanstack/react-query-devtools@4.3.0-beta.3/build/umd/index.production.js I can see this is also visible in bundlephobia: 4.2.1 has |
@TkDodo I would argue that devtools should actually bundle those. Without the need of importing additional intermediate dependencies. What do you think? The other issue is that |
@DamianOsipiuk I generally agree with that, however, the production devtools should effectively be "empty". The Devtools and DevtoolsPanel is just a function that returns
Ha, okay :) |
Oh, right. They need to be bundled in |
any idea what to do about the production devtools lazy import? |
Not yet, but i will look into it. |
testing with
however, that import path is not great. Could we somehow alias that to
I can see the separate bundle being created, and it also gets loaded in the browser network tab when I do looked into this for a bit now, it seems to be something with our query/packages/react-query-devtools/src/utils.ts Lines 103 to 115 in 0fda41a
the ref is never set to true, therefore, we render nothing. Don't know why yet...
|
I can reintroduce that export, but it will only work with bundlers that fully supports exports, like vite.
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; |
if that's fine with you, i think its okay.
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. |
@TkDodo Edit. It does not work with So it seems, when you change React[isServer ? 'useEffect' : 'useLayoutEffect'](() => { to
or
It starts working again... |
@DamianOsipiuk that's an interesting find! we need the - export const isServer = typeof window === 'undefined'
+ export const isServer = () => typeof window === 'undefined'
- React[isServer ? 'useEffect' : 'useLayoutEffect'](() => {
+ React[isServer() ? 'useEffect' : 'useLayoutEffect'](() => { ? |
@DamianOsipiuk I've switched to only using 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? |
@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. |
@TkDodo Typescript issue will go away if you change the And yes, i think that was the last issue. |
I added this paragraph, is that about right? |
Yup, although i think that TS 4.7 is required. |
I got 4.5 from their docs: |
I was more thinking about this https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing 😉 |
okay, I updated it: 40c3491 |
@TkDodo 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) |
That's more something for @ardeora |
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:
The text was updated successfully, but these errors were encountered: