adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.46k stars 1.08k forks source link

Client-only breaks Next.js build #5876

Open thedevdavid opened 6 months ago

thedevdavid commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

Please read the description of this PR for the reasoning: https://github.com/adobe/react-spectrum/pull/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 commented 6 months ago

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

jpgilchrist commented 5 months ago

@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 commented 3 months ago

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).