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

feat(client components): add use client directive at the top of files having client components #4738

Merged
merged 9 commits into from
Jan 24, 2023

Conversation

girishsontakke
Copy link
Contributor

This PR is addressing the improvement suggested in #4689.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 2, 2023

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 a28c5a0:

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

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think there are still a couple of places missing like:

  • useIsFetching
  • useIsMutating
  • the devtools
  • the react-query-persist package

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 13, 2023

not sure where all the conflicts are coming from. can you update with main once more please?

@girishsontakke
Copy link
Contributor Author

girishsontakke commented Jan 14, 2023

not sure where all the conflicts are coming from. can you update with main once more please?

Sure, I have removed the conflicts.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2023

Codecov Report

Base: 96.36% // Head: 91.25% // Decreases project coverage by -5.11% ⚠️

Coverage data is based on head (a28c5a0) compared to base (eab6e2c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4738      +/-   ##
==========================================
- Coverage   96.36%   91.25%   -5.11%     
==========================================
  Files          45      110      +65     
  Lines        2281     4116    +1835     
  Branches      640     1057     +417     
==========================================
+ Hits         2198     3756    +1558     
- Misses         80      339     +259     
- Partials        3       21      +18     
Impacted Files Coverage Δ
src/react/utils.ts
src/react/useMutation.ts
src/core/logger.ts
src/core/mutationCache.ts
src/core/mutationObserver.ts
src/core/onlineManager.ts
src/react/useIsMutating.ts
src/devtools/styledComponents.ts
src/devtools/useLocalStorage.ts
src/react/QueryErrorResetBoundary.tsx
... and 145 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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are more things that should be client components. Basically, everything that uses state, effects or context needs to be a client component. I haven't looked at the full list, but:

  • hydrate.tsx is definitely a client component
  • isRestoring.tsx` has react context

@Ephem
Copy link
Collaborator

Ephem commented Jan 20, 2023

Thanks for working on this!

I finally got around to do some testing, and I actually think we should not include 'use client' at the top of hooks-only files. First of all, it doesn't really have an effect because the components that use the hooks still needs to include the directive.

Second, having it in the hooks-only files actually hurts in a sense. If you forget to add 'use client' to a component that uses one of these hooks (will be a very common mistake in userland), the default is that you get a nice error message from React:

image

But if you have 'use client' in the hooks file, you instead get an unreadable build error (I guess because the file is still removed from the SC-bundle):

image

@TkDodo I think this also means we might want to split any file that has both client components and hooks into two separate ones, like Hydrate.tsx and QueryClientProvider.tsx? So you can use the component as a client component, but still get a nice error message if you try to say use useQueryClient in a nested Server Component somewhere? I think for this PR it's probably fine to just do 'use client' at the top of those files and live with a bad worse error message for a while though, it's all pretty experimental anyway. 😄

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 20, 2023

I think this also means we might want to split any file that has both client components and hooks into two separate ones, like Hydrate.tsx and QueryClientProvider.tsx?

oh yeah, good idea. I think in v5 the Hydrate is no longer a problem because we have removed the hook already, but it makes sense for QueryClientProvider

@Ephem
Copy link
Collaborator

Ephem commented Jan 24, 2023

I took the liberty of removing the 'use client' from the hooks files again, but didn't split the files, I think we can do that later. I think isRestoring.tsx is another annoying candidate btw:

'use client'
import * as React from 'react'

const IsRestoringContext = React.createContext(false)

export const useIsRestoring = () => React.useContext(IsRestoringContext)
export const IsRestoringProvider = IsRestoringContext.Provider

Any such context/useContext pattern will need to be split into two files to get good error messages for the useContext part..

@Ephem Ephem changed the title perf(client components): added use client directive at the top of files having client components feat(client components): add use client directive at the top of files having client components Jan 24, 2023
@Ephem Ephem merged commit f57c8dc into TanStack:main Jan 24, 2023
@girishsontakke girishsontakke deleted the issue-4689 branch January 24, 2023 12:14
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.

None yet

4 participants