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

Client-only breaks Next.js build #5876

Open
thedevdavid opened this issue Feb 14, 2024 · 12 comments
Open

Client-only breaks Next.js build #5876

thedevdavid opened this issue Feb 14, 2024 · 12 comments

Comments

@thedevdavid
Copy link

Provide a general summary of the issue here

#5826 breaks custom components in Next.js

Using composeRenderProps as a util does not work anymore. The below, previously working pattern throws a build error.

import { composeRenderProps } from 'react-aria-components';
import { twMerge } from 'tailwind-merge';
import { tv } from 'tailwind-variants';

export const focusRing = tv({
  base: 'outline outline-blue-600 dark:outline-blue-500 forced-colors:outline-[Highlight] outline-offset-2',
  variants: {
    isFocusVisible: {
      false: 'outline-0',
      true: 'outline-2',
    },
  },
});

export function composeTailwindRenderProps<T>(
  className: string | ((v: T) => string) | undefined,
  tw: string
): string | ((v: T) => string) {
  return composeRenderProps(className, (className) => twMerge(tw, className));
}

🤔 Expected Behavior?

App should build

😯 Current Behavior

./src/lib/utils.ts
'client-only' cannot be imported from a Server Component module. It should only be used from a Client Component.

The error was caused by importing 'react-aria-components/dist/import.mjs' in './src/lib/utils.ts'.

Import trace for requested module:
  ./src/lib/utils.ts
  ./src/app/(site)/layout.tsx

💁 Possible Solution

Need to create 2 separate files for utils. 1 of them should be marked with "use client".

'use client';

import { composeRenderProps } from 'react-aria-components';
import { twMerge } from 'tailwind-merge';

export function composeTailwindRenderProps<T>(
  className: string | ((v: T) => string) | undefined,
  tw: string
): string | ((v: T) => string) {
  return composeRenderProps(className, (className) => twMerge(tw, className));
}
import { tv } from 'tailwind-variants';

export const focusRing = tv({
  base: 'outline outline-blue-600 dark:outline-blue-500 forced-colors:outline-[Highlight] outline-offset-2',
  variants: {
    isFocusVisible: {
      false: 'outline-0',
      true: 'outline-2',
    },
  },
});

🔦 Context

No response

🖥️ Steps to Reproduce

https://codesandbox.io/p/devbox/spring-http-yq2y79

Version

v1.1.0

What browsers are you seeing the problem on?

Other

If other, please specify.

No response

What operating system are you using?

macOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@devongovett
Copy link
Member

devongovett commented Feb 14, 2024

I think field.tsx should have "use client". You shouldn't need to create two files. The reason for this is that composeRenderProps returns a function, so it cannot be passed to a client component (props must be serializable). Here's a working example: https://codesandbox.io/p/devbox/spring-http-forked-cdpsny.

@thedevdavid
Copy link
Author

thedevdavid commented Feb 15, 2024

Sure. But the build issue would still come up if there's anything else in the utils file and I want to import that in a server component. In this example:

lib/utils.ts

import { clsx } from 'clsx';
import { twMerge } from 'tailwind-merge';
import { composeRenderProps } from 'react-aria-components';
....

export function cn(...inputs: ClassValue[]) {
  return twMerge(clsx(inputs));
}

app/layout.tsx

import { cn } from '../lib/utils';

Updated my example: https://codesandbox.io/p/devbox/spring-http-yq2y79

@jpgilchrist
Copy link

jpgilchrist commented Feb 19, 2024

@devongovett this is semi related, semi not related, but I didn't want to open a new issue for this. I'd be glad to open a new issue if you think it is merited.

We also ran into some issues after the recent release. I think the PR was overly aggressive making the entire package client-only. There are components within the package that I feel are appropriate to be used server side. For instance, general layout components such as Flex, Grid, View, Well. Or things like Label, Link, etc. that you actually may want rendered server side.

I totally understand the point of that PR and agree with the rationale. I just think it was aggressive. Should client-only be removed from the root index file and instead moved into components that are actually client-only? I.e., things with handlers such as onPress, etc. should explicitly have 'client-only' defined.

Thoughts?

@devongovett
Copy link
Member

All React Spectrum components are client components because they consume from contexts (e.g. theme, i18n, etc.). Context is not available in server components. But please note that client components are still rendered on the server via traditional SSR to HTML, and then hydrate on the client. This is totally normal and expected. RSCs are for things that only run on the server, like reading from a database. No React Spectrum or React Aria components fall into this bucket.

@jpgilchrist
Copy link

jpgilchrist commented Feb 21, 2024

@devongovett yes, I completely understand what you're saying. However, instead of all components now being marked as 'use client' they are now import 'client-only'.

If they were 'use client' then they'd generally work as expected even if they were interleaved inside of server pages. Now, however, we are forced to wrap any and all spectrum components in it's own 'use client' component for some things that don't feel like they need it.

const Page = async () => {
  const response = await fetch(...)
  return (
    <Flex>
         <Well>...</Well>
    </Flex>
  )
}

as an example now is not possible as it throws the client-only exception. It's not like it's the end of the world, I guess we can all add a wrapping client component and pass children in it as well in order to accomplish the same things. I am generally under the impression that some packages absolutely should be 'client-only' such as things that take functions as props as I alluded to previously (TS will throw a warning here anyway). But things that don't violate those prop conditions, then I don't see why they couldn't be left as 'use client'.

In our case, it was a bit of a breaking change, but we'll adapt accordingly.

@jpgilchrist
Copy link

jpgilchrist commented Feb 23, 2024

@devongovett actually we went to try and refactor some code to work with the latest version and it's a no go on our end. We have so many async pages in nextjs that utilize Flex, Well, View, Breadcrumbs, etc. that it will be incredibly painful and time consuming.

I would strongly request you reconsider your position here. However, I understand that I am just one person with one use case and I don't speak for everyone.

@laurens-mesure
Copy link

I'm also experiencing the same issue. I think the move to "client-only" was a maybe bit too much. What were the issues with just using "use client"?

@devongovett
Copy link
Member

Please read the description of this PR for the reasoning: #5826. There aren't many components that would realistically work on the server, e.g. have no event handlers at all. @jpgilchrist Flex, Well, and View are pretty much the only ones. Even Breadcrumbs has events. In addition, the way we currently bundle components means that we can't just add client-only to some components but not others. So given all that I'm not sure it's really worth the hassle of supporting and documenting the inconsistency for those few components that might work inside a server component.

@laurens-mesure
Copy link

After reading the pull request I understand why this change was made. Thanks!

@jpgilchrist
Copy link

@devongovett I understand. I'm just a little frustrated given that we interleaved the acceptable components (Well, Flex, View, etc.) within our server components and correctly used the event based components that use event handlers, context, etc. within wrappers. But given the structure of some of our server pages, this is just a very painful experience. For us it's a breaking change.

@javascripter
Copy link

javascripter commented May 11, 2024

Link, Breadcrumbs, Tabs, Tooltip, etc have legitimate use cases without event handlers I think.

With 'client-only', we cannot do this directly. Considering Link is just an accessible <a> component replacement, not being able to use them without a wrapper feels a bit cumbersome.

// RSC component
export default function Page() {
  return <div>
    <Link href="/some-page" />
  </div>
}

Even for components like Button and Dialog, where using event handlers may be more common, there are cases where usage without event handlers are normal.

For example, one may extract Modal component into use client components, and may want to leave DialogTrigger and Button directly in RSC components. Considering RSC allows composition of server and client components, this kind of partial extraction of client components are somewhat common.

// RSC component
export default function Page() {
  return <div>
    <DialogTrigger>
      <Button>Sign up…</Button>
      <SignUpModal />
    </DialogTrigger>
  </div>
}

// SignUpModal.tsx
'use client'

export function SignUpModal() {
  return <Modal>
    <Dialog>
      {({ close }) => (
        <div>
          <Heading slot="title">Sign up</Heading>
          ...
        </div>
      )}
    </Dialog>
  </Modal>
}

Considering the above, I think passing serializable props to React Aria Components should be the responsibility of the application developers rather than a strict enforcement from the library?

React Docs also seems to assume use client usage for third party UI components with no mention about client-only.
I also expect users to be aware they cannot pass event handlers as part of the RSC contract/limitations anyway (the error message is not that great but it's a Next.js implementation issue I guess).

@Ibrahim-Achkar
Copy link

Ran into this today, not a big hassle to wrap the component but makes me wonder whether it will have negative impacts on my patterns and app structure - will keep going to see what the effect is. The library itself is 🌟 so it's worth the hassle.

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

No branches or pull requests

6 participants