search

storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
83.95k stars 9.22k forks source link

[Bug]: RSC stories that use "react-server" exports do not get bundled correctly #27527

Open shawngustaw opened 3 months ago

shawngustaw commented 3 months ago

Describe the bug

Hi there! 👋

Apollo has a package for integrating streaming + apollo client found here: https://github.com/apollographql/apollo-client-nextjs/

Recently they migrated to react-server conditional exports (RFC found here: https://github.com/reactjs/rfcs/blob/main/text/0227-server-module-conventions.md#react-server-conditional-exports) such that code that's supposed to only run on the server never gets imported in a client bundle. This obviously breaks in storybook where the RSC implementation is actually running in the browser. Does anyone have any suggestions around how to make this work? I expect more and more packages are going to use react-server for exports/bundling so it feels like this is something storybook should be prepared to handle.

I provided a simple repo below that could be a starting point for coming up with a fix - it's just a barebones Next app with storybook 8 installed.

Reproduction link

https://github.com/shawngustaw/storybook-rsc-repro

Reproduction steps

  1. npm i
  2. npm run dev
  3. verify Pokemon component renders correctly in NextJS app router using RSC's
  4. npm run storybook
  5. verify the same does not run within storybook

System

Storybook Environment Info:

  System:
    OS: macOS 14.4.1
    CPU: (12) arm64 Apple M2 Pro
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.0 - ~/.nvm/versions/node/v18.17.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v18.17.0/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.0/bin/npm <----- active
    pnpm: 9.1.4 - ~/.nvm/versions/node/v18.17.0/bin/pnpm
  Browsers:
    Chrome: 125.0.6422.61
    Safari: 17.4.1
  npmPackages:
    @storybook/addon-essentials: ^8.1.5 => 8.1.5 
    @storybook/addon-interactions: ^8.1.5 => 8.1.5 
    @storybook/addon-links: ^8.1.5 => 8.1.5 
    @storybook/addon-onboarding: ^8.1.5 => 8.1.5 
    @storybook/blocks: ^8.1.5 => 8.1.5 
    @storybook/nextjs: ^8.1.5 => 8.1.5 
    @storybook/react: ^8.1.5 => 8.1.5 
    @storybook/test: ^8.1.5 => 8.1.5 
    eslint-plugin-storybook: ^0.8.0 => 0.8.0 
    storybook: ^8.1.5 => 8.1.5

Additional context

I brought this up with the Apollo team here: https://github.com/apollographql/apollo-client-nextjs/issues/307 No response

shilman commented 3 months ago

@kasperpeulen @valentinpalkovic @ndelangen can we get storybook to bundle react-server code into our browser bundles by default?

kasperpeulen commented 3 months ago

Thanks for the detailed issue! @shawngustaw

For now I can recommend the following work-around:

// main.ts
  webpackFinal: (config) => {
    config.resolve?.conditionNames?.unshift('react-server')
    config.resolve!.alias = {
      ...config.resolve?.alias,
      'client-only': false
    }
    return config;
  }
image

I'm going to investigate how to best solve this in general. It is kind of tricky. In storybook, we do run the async RSC components as async client components. And we recommend people to use module mocking to avoid running node specific code in the browser.

This is our RSC demo that uses an in-memory database in storybook: https://github.com/storybookjs/storybook-rsc-demo/blob/6cca874e7736d70d36d770b42dfc6ccf98ce125a/package.json#L12-L15

Running both the server and client stuff in one environment (the browser) has a lot of advantages as it comes to a true unit-testing experience. It is super fast, and you can both set a server mock, spy on a function and access a dom element in the same unit/component test: https://github.com/storybookjs/storybook-rsc-demo/blob/main/app/note/edit/page.stories.tsx

My feeling is that the best thing we can do, is trick the bundler, to think of storybook as both a react-server and a react-client in the same time, which is what the above tries to achieve.

shilman commented 3 months ago

@kasperpeulen Thanks so much for tracking that down. If we can't safely apply this configuration automatically by default, at least we can add that configuration (and its Vite equivalent) prominently in our RSC docs once that's a thing. cc @kylegach

kasperpeulen commented 3 months ago

@shilman After thinking a bit more about it. Maybe we should just do add it to the nextjs framework by default when experimentalRSC: true is set to the 8.2 alpha release. And see what it breaks.

I can imagine that in theory this does unwanted stuff, for example, when a library exports both a RSC and client variant of the same component. I only don't know if this is really what the ecosystem is gonna do, or that this is just an imaginary example.

In this worst case scenario, I think we should recommend people to have 2 storybooks. One that is for RSC components, and one for client only components.

shilman commented 3 months ago

Good call @kasperpeulen ! @shawngustaw is that a change you might be able to contribute?

BradMcGonigle commented 1 month ago

I can imagine that in theory this does unwanted stuff, for example, when a library exports both a RSC and client variant of the same component. I only don't know if this is really what the ecosystem is gonna do, or that this is just an imaginary example.

I can provide two examples (I'm sure there are more and will be many more in the future) that do this type of exporting. Our team has been trying work around issues for a few days because we rely on these packages but also want to have storybook coverage for components/pages that use them.

With the recommended work-around these packages throw warnings for exports that are not found and stories with components using these imports fail to render with webpack import errors.

WARN export 'useForm' (imported as 'useForm') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'useForm' (imported as 'useForm') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'FormProvider' (imported as 'FormProvider') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'Controller' (imported as 'Controller') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'useFormContext' (imported as 'useFormContext') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'useForm' (imported as 'useForm') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'useForm' (imported as 'useForm') was not found in 'react-hook-form' (possible exports: appendErrors, get, set)
WARN export 'ApolloNextAppProvider' (imported as 'ApolloNextAppProvider') was not found in '@apollo/experimental-nextjs-app-support' (possible exports: ApolloClient, DebounceMultipartResponsesLink, InMemoryCache, RemoveMultipartDirectivesLink, SSRMultipartLink, built_for_rsc, registerApolloClient)
kasperpeulen commented 1 month ago

@BradMcGonigle It is quite a tricky situation, and hard to handle in a generic way.

It seems that it might be best to not use:

config.resolve?.conditionNames?.unshift('react-server')

Another workaround would be:

  webpackFinal: (config) => {
    config.resolve!.alias = {
      ...config.resolve?.alias,
      "@apollo/experimental-nextjs-app-support/rsc$": path.resolve(
        __dirname,
        "../node_modules/@apollo/client-react-streaming/dist/index.rsc",
      ),
    };
    return config;
  },

And then always import registerApolloClient from the rsc subpath:

import { registerApolloClient } from "@apollo/experimental-nextjs-app-support/rsc";

This will make sure it works both in storybook and in the live production.

@shilman We should probably contact Apollo, and discuss better solutions.

phryneas commented 1 month ago

We will remove the RSC subpath in the next release, as we want to simplify this for our normal users 😞

I think the people to contact here are probably the React team, since they are the ones who have a working "dual land" webpack plugin that can load files twice in both contexts. Maybe they have an idea (they came up with the whole thing after all).

kasperpeulen commented 1 month ago

@phryneas

as we want to simplify this for our normal users

I'm not sure if that is really more simple. If you import @apollo/experimental-nextjs-app-support, TS thinks that all exports are available, and TS won't complain if you use them, even though in fact they will be only conditionally available.

Splitting the exports into different import subpaths seems safer and more straight-forward to me, even outside of the storybook context.

I think the people to contact here are probably the React team, since they are the ones who have a working "dual land" webpack plugin that can load files twice in both contexts.

The problem is that there is only one context in storybook. The browser. We never run RSC in the server. And async components work similarly in the browser and the server. They are not recommended to run in the browser for production use cases, but the React team says, they might support that in the future as well:

In the future, we could unlock general support for async Client Components by compiling them to a specialized, generator-like runtime. https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md#why-cant-client-components-be-async-functions

We belief that running async components in the browser gives the most straigt-forward unit testing experience, without having to setup a server. We have plan to polyfill the Node API's in the browser, to make this experience easier, and have recipes to mock out the database with an in-memory database.

Having two different systems, a browser and a server, puts you really into the E2E kind of space. Which is very useful, but with different tradeoffs. For example, you lose the control and flexibility you have in unit testing, where you can just access any variable and call any function/Component. If you are gonna mock or spy in a unit test. Are you importing the mock from the server or the browser? Do you import the Date global from the server or the browser etc.

We believe that for a storybook, where you develop/test and document components in isolation, having only to deal with one runtime makes the most sense.

If you belief (and other libraries belief) that having both RSC and browser exports in one sub-path, is the best pattern. Perhaps, we could up with an ecosystem convention.

Add a dual react-server/browser export:

  "exports": {
    ".": {
      "import": {
        "types": "./dist/combined.d.ts",
        "react-server": {
          // for combined RSC/browser environments like storybook, possibly vitest browser mode as well
          "browser":  "./dist/combined.js",
          "default":  "./dist/index.rsc.js",
        },
        "edge-light": "./dist/index.ssr.js",
        "browser": "./dist/index.browser.js",
        "node": "./dist/index.ssr.js"
      }
    },

If you agree, I can make a PR for that.

phryneas commented 1 month ago

I'm not sure if that is really more simple. If you import @apollo/experimental-nextjs-app-support, TS thinks that all exports are available, and TS won't complain if you use them, even though in fact they will be only conditionally available.

The same is true for the react package itself, where some exports like useState are available in Client Components, but crash the bundler for Server Components (so during build time, long before run time - just like they would with the Apollo Client RSC package). We are really following React's conventions here that they have intended to be used in the ecosystem.

As for the "more simple" part: a lot of the exports are available in all variants, but with different implementations per condition.

Previously, we had users import from /rsc in RSC and from /ssr in SSR. It turned out to be extremely confusing for them, and lead to many copy-paste errors that I've spotted in issues over the time. Each of those "wrong imports" lead to bugs or the library flat out not working as expected. It is really a lot less prone to errors if they are available under the same export and just swap for the right implementation transparently.

We never run RSC in the server.

You can RSC in the browser as well, using the RSC build of React, and then send the result to the SSR build of React, and then hydrate it with the Browser build of React - all three steps in the same browser. (Preferrably in separated workers) They are not bound to a server, despite the name. They already run in all kind of edge runtimes, which are often much closer to a browser than node.

I'm really sorry, I know you deserve a longer answer, but it's very late here and I'm very tired.

Generally, it boils down to this: This is a pattern that the React team intends to be used throughout the ecosystem and that they are modelling themselves.

I don't want Apollo Client to become the place this is fought out on. We had enough discussions around bundling with the React and Next.js team over the last year. If you have complaints about this pattern, please take them up to the React team. We are just one library caught in the middle that is a bit faster in implementation than many others, but many others will follow once React 19 officially releases and you'll have this fight over and over again, until you either convince the React team that this is not a good pattern (to be honest, at this point it's probably years too late for having that discussion, see the RFC linked in the first comment) or actually run RSCs with the RSC build in the browser - which the React team can probably help you with.

PS: Rereading this a bit more:

I see where you are coming from with the idea of "only one runtime" - but running RSC code and Browser code in the same environment/scope seems very dangerous to me. What will window be? undefined or globalThis? You'd run the risk of leaking data that would usually be separated by processes through a now-shared global scope. Your storybook stories would be far removed from "real" behaviour, having bugs that can only occur in that environment, and hiding bugs that would occur in a real-life situation. Yes, you will lose the simplicity of unit testing only a single environment. But that's because RSC are not simple. Anyone who starts using RSC will need to understand that they write code for three different environments, and as a result they will also need to wrap their head around writing tests for these three environments. This is something where you can innovate testing patterns - but please keep the real constraints in place. Nobody will be happy if they have amazing Storybook stories that can never work that way in a real application.

The idea of the "combined" export would really be an export just for storybook and we'd have to write a completely separate implementation that ignored normal React behavior to work under the constraints you create in StoryBook that are not shared by any real-life deployment of a React application. You can't expect every React library to do that 😕

PPS: one more example where this is dangerous:

React.cache is available in both environments, but behaves completely different - it just doesn't do anything in the browser, which will completely confuse a library that is written against the assumption to run in RSC, where it memoizes method calls. We use that in our RSC code - and we rely on the memoization. Userland code using registerApolloClient with the wrong React.cache in the background will create new Apollo Client instances all over again, instead of just creating one for the whole request. It will start over with an empty cache every time, and in the best case just make a ton of extra network requests and in the worst case completely fail because of missing cache contents.

BradMcGonigle commented 1 month ago

Like @phryneas I think tackling this from the React side is probably the only real "solution" without playing wack-a-mole as more packages adopt react-server conditional exports.

If there are discussion with the the React team please reference this issue because I'd love to follow along and contribute to the discussion if possible.

BradMcGonigle commented 1 month ago

And then always import registerApolloClient from the rsc subpath:

import { registerApolloClient } from "@apollo/experimental-nextjs-app-support/rsc";

This will make sure it works both in storybook and in the live production.

@yannbf importing registerApolloClient this way is actually called out as being deprecated so not sure it's a work around that makes sense here.

phryneas commented 1 month ago

I might be missing a part of the conversation here, but as I said before, running registerApolloClient with react-dom/index.js will always result in bugs.

It expects to run in react-dom/react-dom.react-server.js, and that entry point has completely different behaviour.

kasperpeulen commented 1 month ago

I have no time at the moment to write an adequate reply.

But I do appreciate your feedback, @phryneas. The point that even react itself is using this convention for loading a different cache with react-server conditions has made us realize that we need to address this "somehow" from the storybook side.

We are still discussing how we can address this, but we are too busy releasing 8.3 at the moment. So I will write a more proper longer reaction later!

devdumpling commented 1 month ago

Just wanted to add a +1 to that, while the sort of mocking of RSC environment in the browser is workable, I think storybook is likely to continue to hit these kinds of issues in the future. It's a tough spot to be in.

Appreciate all the thought and discussion happening here. I think @BradMcGonigle and I may have a workaround that can keep us afloat for a bit, but agree it feels like a more ideal solve is to handle the conditional export more explicitly.