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

[Bug]: Using appDirectory in preview.ts causes reactdom.preload error with Next.js framework #23661

Closed
terrymun opened this issue Jul 31, 2023 · 5 comments · Fixed by #23671
Closed

Comments

@terrymun
Copy link

terrymun commented Jul 31, 2023

Describe the bug

When using the image component from next/image, adding the priority flag will cause storybook stories to fail to render if the following conditions are met:

Drilling down further, it seems that it is the addition of appDirectory: true that is causing the failure:

Screenshot 2023-07-31 at 10 46 23

If I remove the appDirectory: true setting, then all is good:

Screenshot 2023-07-31 at 10 50 32

However, I cannot remove this configuration in my production storybook as any components that are using useRouter from next/navigation will no longer render.

To Reproduce

A simple GitHub repo to reproduce this issue is available here: https://github.com/terrymun/storybook-nextjs-image-with-priority

An alternative way to reproduce this is to add the following lines to the preview.js (or preview.ts) file on a Storybook that is configured to work with Next.js:

export const parameters = {
  // ... your other parameters config

  nextjs: {
    // WARNING: This will cause the storybook to throw an error
    appDirectory: true,
  },
}

System

Environment Info:

  System:
    OS: macOS 13.4.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 18.16.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.5.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 115.0.5790.114
    Edge: 115.0.1901.188
    Safari: 16.5.2
  npmPackages:
    @storybook/addon-essentials: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/addon-interactions: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/addon-links: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/addon-onboarding: ^1.0.8 => 1.0.8 
    @storybook/blocks: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/nextjs: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/react: ^7.2.0-rc.0 => 7.2.0-rc.0 
    @storybook/testing-library: ^0.2.0 => 0.2.0

Additional context

No response

@sookmax
Copy link
Contributor

sookmax commented Jul 31, 2023

@terrymun Hi, this seems like a bug introduced by next@13.4.11 (related issue: vercel/next.js#53272).

I tested that it works if I downgrade nextjs to 13.4.10.

The problematic import { preload } from 'react-dom' was introduced by this PR (vercel/next.js#52425). And it seems like ReactDOM.preload() is not part of the public APIs yet (no type definition found in @types/react-dom and no related entry in react docs). Looks like it's a new API added fairly recently: facebook/react#26940

@terrymun
Copy link
Author

terrymun commented Jul 31, 2023

Thanks for digging into the changes that caused it @sookmax. We have implemented an interim fix for this: which is to simply shim next/image in our storybook webpack config:

export const config: StorybookConfig = {
  // Your config goes here...

  // Webpack config to define custom alias for next/image
  webpackFinal: async (config) => {
    if (config.resolve) {
      config.resolve = {
        ...config.resolve,
        alias: {
          ...config.resolve.alias,
          /*
           * NOTE: The next-image-shim manually disables the `priority` prop due to a bug with storybook
           *       https://github.com/storybookjs/storybook/issues/23661
           * TODO: Remove this shim once the issue has been resolved
           */
          'next/image': path.resolve(__dirname, 'next-image-shim.js'),
        },
      };
    }
  },
};

The known solution of using Object.defineProperty to override next/image stopped working in the more recent versions of storybook, so we composed a shim instead that looks like this:

// NOTE: Import from node_modules directly otherwise webpack will enter a recursive loop when aliasing next/image
// eslint-disable-next-line no-restricted-imports
import Image from '../../../node_modules/next/image';

/*
 * NOTE: The next-image-shim manually disables the `priority` prop due to a bug with storybook
 *       https://github.com/storybookjs/storybook/issues/23661
 * TODO: Remove this shim once the issue has been resolved
 */
const NextImage = (props) => <Image {...props} unoptimized priority={false} />;

export default NextImage;

I hope this will be helpful to whoever that comes across this issue :)

@sookmax
Copy link
Contributor

sookmax commented Aug 1, 2023

Warning ⚠️

Don't use this workaround yet. It's going to break your docs pages 😅. Sorry about that.


@terrymun Hey I dug a little deeper on this matter, and it looks like next's webpack is configured to use pre-compiled react and react-dom that are embedded in next package (next/dist/compiled/react and next/dist/compiled/react-dom) when it is compiling files in app/: https://github.com/vercel/next.js/blob/ff5338ce03a3240a97a5c84f5ad5c31c0f53a6ce/packages/next/src/build/webpack-config.ts#L1370-L1379

I speculate the pre-compiled react and react-dom that next uses is either canary or experimental version of React. The pre-compiled react-dom in particular, indeed contains a function named preload in it: https://github.com/vercel/next.js/blob/ff5338ce03a3240a97a5c84f5ad5c31c0f53a6ce/packages/next/src/compiled/react-dom/cjs/react-dom.development.js#L36524-L36533

Unfortunately, current storybook's webpack config in @storybook/nextjs doesn't seem to know about this, so it uses react-dom that the user installed, which doesn't include preload function yet, hence the error in your screenshot.

I'm planning to submit a PR for this issue, but in the meantime, could you try using the webpack aliases below instead of your workaround and see if it solves the issue?

// .storybook/main.ts
import type { StorybookConfig } from '@storybook/nextjs';
import path from 'path';

const config: StorybookConfig = {
  // Your config goes here...

  webpackFinal: async (config) => {
    if (config.resolve) {
      config.resolve = {
        ...config.resolve,
        alias: {
          ...config.resolve.alias,
          alias: {
              ...config.resolve?.alias,
              'react': path.dirname(require.resolve('next/dist/compiled/react')),
              'react-dom': path.dirname(require.resolve('next/dist/compiled/react-dom')),
           }
        },
      };
    }
  },
};

export default config;

In my local environment, it seems to work with above config, but I wanted to know if the same is true for your case.

Thanks.

kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Aug 8, 2023
…#53443)

### What?

`ReactDOM.preload` is available in `react-dom@experimental` builds. If it's not available, we should fall back to `Head`+`link`

### Why?

Since `ReactDOM.preload` is only available in `react-dom@experimental` builds, certain environments (like Jest or [Storybook](storybookjs/storybook#23661)) might have a version of `react-dom` installed that won't work with `preload()`

### How?

Closes NEXT-1482
Fixes #53272

See also: storybookjs/storybook#23661

Co-authored-by: Steven <229881+styfle@users.noreply.github.com>
@balazsorban44
Copy link
Contributor

Hi, this should not be the problem anymore, since vercel/next.js#53443 was merged! Cheers!

@JReinhold
Copy link
Contributor

@terrymun @sookmax can you confirm that the issue is actually solved now, with the upstream change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants