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

Preserve 'use client' directives #5161

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

Ephem
Copy link
Collaborator

@Ephem Ephem commented Mar 19, 2023

Fixes #4933

This makes everything work as documented with Next app router, that is, developers should no longer need to:

  • Wrap our imports in "use client" themselves (just import and use Hydrate directly for example)
  • Import from @tanstack/query-core in RSCs (can now import directly from @tanstack/react-query)

For anyone reading along: Note that React Query-support for the Next app router is still experimental!

I tested it like this (basically quick version of the docs):

// Providers.jsx
// This wraps the entire app in root layout.jsx
"use client";

import { useState } from "react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";

export default function Providers({ children }) {
  const [queryClient] = useState(() => new QueryClient());

  return (
    <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
  );
}
// page.jsx
import { dehydrate, Hydrate, QueryClient } from "@tanstack/react-query";
import ClientComponent from "./ClientComponent";

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

  await queryClient.fetchQuery({
    queryKey: ["1"],
    queryFn: () => "Server",
  });

  return (
    <Hydrate state={dehydrate(queryClient)}>
      <ClientComponent />
    </Hydrate>
  );
}
// ClientComponent.jsx
"use client";

import { useQuery } from "@tanstack/react-query";

export default function ClientComponent() {
  const { data } = useQuery({
    queryKey: ["1"],
    queryFn: () => "Client",
    // Set staleTime to see that we get the "Server" output,
    // can later switch tabs to trigger revalidation and see we
    // get the "Client" output
    staleTime: 10000,
  });

  return <div>{data}</div>;
}

@vercel
Copy link

vercel bot commented Mar 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
query ⬜️ Ignored (Inspect) Mar 19, 2023 at 3:45PM (UTC)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5c925aa:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 97.67% and project coverage change: +0.31 🎉

Comparison is base (ed7d9f8) 91.90% compared to head (5c925aa) 92.22%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5161      +/-   ##
==========================================
+ Coverage   91.90%   92.22%   +0.31%     
==========================================
  Files         111      111              
  Lines        4188     4229      +41     
  Branches     1083     1099      +16     
==========================================
+ Hits         3849     3900      +51     
+ Misses        318      308      -10     
  Partials       21       21              
Impacted Files Coverage Δ
packages/query-core/src/query.ts 99.03% <ø> (ø)
...kages/react-query-devtools/src/styledComponents.ts 92.85% <ø> (ø)
packages/react-query/src/errorBoundaryUtils.ts 100.00% <ø> (ø)
packages/react-query/src/reactBatchedUpdates.ts 100.00% <ø> (ø)
packages/react-query/src/useBaseQuery.ts 95.45% <ø> (ø)
packages/react-query/src/useInfiniteQuery.ts 100.00% <ø> (ø)
packages/react-query/src/useIsFetching.ts 92.30% <ø> (ø)
packages/react-query/src/useIsMutating.ts 92.30% <ø> (ø)
packages/react-query/src/useMutation.ts 96.15% <ø> (ø)
packages/react-query/src/useQueries.ts 94.64% <ø> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 20, 2023

I've checked the output of the codesandbox preview build:

  • /build/umd/index.development.js doesn't have any 'use client'
  • /build/umd/index.production.js doesn't have any 'use client'
  • /build/lib/Hydrate.esm.js has 'use client' at the top
  • /build/lib/Hydrate.js has 'use client' at the top, followed by 'use strict'

This looks like what we want 👍 . Shipping it 🚢

@TkDodo TkDodo merged commit 017867e into TanStack:main Mar 20, 2023
@thebuilder
Copy link
Contributor

I'm seeing a lot of warnings like this:

Module level directives cause errors when bundled, "use client" in "node_modules/.pnpm/@tanstack+react-query@4.28.0_biqbaboplfbrettd7655fr4n2y/node_modules/@tanstack/react-query/build/lib/useQueries.mjs" was ignored.

Occurs when bundling client output with Vite. Not sure what I need to change in my setup, to support the "use client" directive?

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 20, 2023

I'm seeing the same warnings in rollup:

packages/react-query/src/useMutation.ts (1:0)
1: 'use client'
   ^
2: import * as React from 'react'
3: import { useSyncExternalStore } from './useSyncExternalStore'
(!) Module level directives cause errors when bundled, 'use client' was ignored.
packages/react-query/src/useInfiniteQuery.ts (1:0)

it does make sense because when we bundle together into one file, we shouldn't keep 'use client'.

@thebuilder
Copy link
Contributor

Won't that always be the case when consuming react-query? It will always be bundled into the application?

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 20, 2023

I don't know. I guess nextJs might consume the .mjs files separately and then decide based on that what is a client component and what isn't. I honestly don't know enough about how next does this 🤷

@thebuilder
Copy link
Contributor

Great - Not just me that's confused about this new "use client" thing. Add it to the pile of problems we already have with CommonJS, ESM and module publishing. :(

Is this the right solution for the the react-query library? If the Next.js 13 app folder is still beta, should the ecosystem figure out the right way of handling use client in libraries as part of this?

@Ephem
Copy link
Collaborator Author

Ephem commented Mar 20, 2023

@thebuilder It is confusing, and sorry for adding a bunch of warnings to your build! I suspect you are not using Server Components at the moment, so you can safely just ignore these warnings.

If you want to hide the annoying output, you can add a Rollup onwarn filter to hide them. I get why Rollup warns for this, but now that directives is becoming more common, it's also unfortunate.

The reason you are seeing this is because you are using the unbundled .mjs-version of React Query, which now includes 'use client' at the top of some files. It's good that you are using this version, because that means your bundler can remove any parts of React Query you are not actively using, so it doesn't get included in your final app-bundle!

Thing is though, Server Components can use this 'use client' directive to do even smarter code splitting. So yes, React Query will always get bundled into the application one way or another in the end, but for the unbundled builds that are a sort of "intermediary step", we want to provide all the "metadata" we can to bundlers to do smart things.

This is all based on a specification any bundler can implement too, so it's not Next.js specific even if they were first to implement it. It's definitely something that is up to us as an ecosystem to figure out, and different libraries might have different tradeoffs.

@Ephem
Copy link
Collaborator Author

Ephem commented Mar 20, 2023

Oh, and just to answer your question specifically to avoid confusion:

Not sure what I need to change in my setup, to support the "use client" directive?

Since you are not using Server Components, you don't need to "support" the directive! Your bundler is warning because it doesn't know what to do with this. It's at the top of a file it wants to include in a bundle, and it can't safely hoist that directive to the entire bundle, so it's removing it instead, which is fine since you don't need it. It doesn't want to just remove things without warning though, which is why you are seeing this.

@thebuilder
Copy link
Contributor

Thanks for the explanation. Since RSC is still only supported in a few select frameworks, like Next, it obviously is not something we can use with Vite or other frameworks.

I feel like this is something that Vite/Rollup should handle out of the box. My build output is going to be filled with warnings, especially if more libraries start adding 'use client'. Until that happens, it seems a bit premature to start adding it to library code?

@Ephem
Copy link
Collaborator Author

Ephem commented Mar 20, 2023

I think about it the other way around though. Unless libraries start using this, what's the incentive to change/fix the behaviour? 😃

Again, sorry about the noisy warnings, adding an onwarn handler is a quick way to get rid of them. If it helps I can put together the specific config you need to add tomorrow?

@thebuilder
Copy link
Contributor

Maybe it should be added directly to the https://github.com/vitejs/vite-plugin-react/tree/main/packages/plugin-react package instead?

@Ephem
Copy link
Collaborator Author

Ephem commented Mar 21, 2023

Absolutely! I only suggest adding the onwarn as a stopgap. I think ultimately Rollup should only output this warning once per build instead of once per file and I think any React plugin that explicitly does not support RSCs could hide that warning by default.

@duypham90
Copy link

I got many error warning when build production with React + Vite v4.2.1
Not use NextJS

Screen Shot 2023-03-31 at 21 09 59

@vadymstebakov
Copy link

vadymstebakov commented Apr 2, 2023

I got many error warning when build production with React + Vite v4.2.1 Not use NextJS

Screen Shot 2023-03-31 at 21 09 59

I got the same issue with React + Vite v4.1.4

@vaynevayne
Copy link

same as vite 4.2.1

@BATCOH
Copy link

BATCOH commented Apr 13, 2023

Here is the vite.config.ts snippet that disables "use client" warnings

import { defineConfig } from "vite";

// https://vitejs.dev/config/
export default defineConfig({
  build: {
    rollupOptions: {
      /**
       * Ignore "use client" waning since we are not using SSR
       * @see {@link https://github.com/TanStack/query/pull/5161#issuecomment-1477389761 Preserve 'use client' directives TanStack/query#5161}
       */
      onwarn(warning, warn) {
        if (
          warning.code === "MODULE_LEVEL_DIRECTIVE" &&
          warning.message.includes(`"use client"`)
        ) {
          return;
        }
        warn(warning);
      },
    },
  },
});

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.

Doesn't work out of the box with next 13
8 participants