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

Storybook example is broken on Next 12.1.5 #36417

Closed
1 task done
LZL0 opened this issue Apr 24, 2022 · 12 comments
Closed
1 task done

Storybook example is broken on Next 12.1.5 #36417

LZL0 opened this issue Apr 24, 2022 · 12 comments
Labels
examples Issue/PR related to examples locked

Comments

@LZL0
Copy link

LZL0 commented Apr 24, 2022

Verify canary release

  • I verified that the issue exists in Next.js canary release

Provide environment information

Windows 10 - 64bit with npm@latest. The issue probably exists on all environments.

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

next/image changes in 12.1.5 broke static image imports of the Storybook example.

Failed to parse src "static/media/public/nyan-cat.png" on next/image, if using relative image it must start with a leading slash "/" or be an absolute URL (http:// or https://)

I've confirmed that the issue exists in 12.1.6-canary.6.

Expected Behavior

Images rendering without an error message.

To Reproduce

npx create-next-app --example with-storybook with-storybook-app
npm run storybook
Navigate to the Nextjs Images Page tab.

@LZL0 LZL0 added the bug Issue was opened via the bug report template. label Apr 24, 2022
@LZL0
Copy link
Author

LZL0 commented Apr 24, 2022

An issue already exists at RyanClementsHax/storybook-addon-next.

@balazsorban44 balazsorban44 added examples Issue/PR related to examples and removed bug Issue was opened via the bug report template. labels Apr 25, 2022
@RyanClementsHax
Copy link
Contributor

RyanClementsHax commented Apr 25, 2022

So it seems that the following code no longer works

// .storybook/preview.js
import * as NextImage from "next/image";

const OriginalNextImage = NextImage.default;

Object.defineProperty(NextImage, "default", {
  configurable: true,
  value: (props) => <OriginalNextImage {...props} unoptimized />,
});

This has been the convention to getting next/image working in storybook for a long time. Granted this is a hack that abuses imports. I'm not sure why it is broken now given that this is a compare between next 12.1.4 and 12.1.5 and nothing stands out to me as a major change.

Was there anything that changed between these two versions that changed the way things were exported?

Unfortunately this isn't just an issue with storybook-addon-next this is likely going to break many nextjs projects using storybook because this method of getting next/image to work is what the vast majority of search results return. It is also what Storybook themselves say to use.

Btw, this method is technically an abuse of module imports in my opinion so I don't think the nextjs team is faulted here at all. I know they're trying to make the best project they can possibly build :)

@rsanchez
Copy link

I did some digging here and found that it works on v12.1.5-canary.6, but errors on v12.1.5-canary.7. I can get canary-7 working by reverting this commit, and in particular, the swc taskfile: da6f271#diff-17f1df42fa9d7a99d7abadac24d7ad226b52616faa1af14b412276d3d598cfe5

@RyanClementsHax
Copy link
Contributor

RyanClementsHax commented May 1, 2022

My guess is that the code it injects for interop (shown below) is what is causing this issue. I will need to inspect further to verify.

if (typeof exports.default === 'function' || (typeof exports.default === 'object' && exports.default !== null)) {
  Object.assign(exports.default, exports);
  module.exports = exports.default;
}

@RyanClementsHax
Copy link
Contributor

After diving deeper into the code I found the root of the issue.

The generated code mentioned above makes the following assignments:

Object.assign(exports.default, exports);
module.exports = exports.default;

For some reason, __esModule: true isn't preserved on module.exports

console.log('before', module.exports.__esModule) // true

if (typeof exports.default === 'function' || (typeof exports.default === 'object' && exports.default !== null)) {
    Object.assign(exports.default, exports);
    module.exports = exports.default;
}

console.log('after', module.exports.__esModule) // undefined

In order for import Image from 'next/image' to get module.exports.default instead of module.exports, __esModule: true needs to be present on module.exports.

This means that the webpack runtime is treating the export from next/image as if it shouldn't be treated as an esm, which is what the code that disables image optimization requires:

import * as NextImage from "next/image";

const OriginalNextImage = NextImage.default;

Object.defineProperty(NextImage, "default", {
  configurable: true,
  value: (props) => <OriginalNextImage {...props} unoptimized />,
});

Consequently, because we aren't disabling image optimization, we are getting a bunch of errors as image optimization doesn't work outside of a nextjs environment.

Therefore, my question to the maintainers is if __esModule: true should be set, thus meaning that this is a bug, or if this is intended behavior and we should come up with a different solution such as the code below:

import * as NextImage from "next/image";

const OriginalNextImage = NextImage.default;

NextImage.default = props =>
  typeof props.src === 'string' ? (
    <OriginalNextImage {...props} unoptimized blurDataURL={props.src} />
  ) : (
    <OriginalNextImage {...props} unoptimized />
  );

NextImage.__esModule = true;

(The above code feels hacky so if anyone has any better approaches, I'm all ears)

@diegohmac
Copy link

Thank you @RyanClementsHax !

This is how it worked for me following Ryan's approach:

import * as NextImage from "next/image";

const OriginalNextImage = NextImage.default;

Object.defineProperty(NextImage, "default", {
  configurable: true,
  value: (props) => typeof props.src === 'string' ? (
    <OriginalNextImage {...props} unoptimized blurDataURL={props.src} />
  ) : (
    <OriginalNextImage {...props} unoptimized />
  ),
});

Object.defineProperty(NextImage, "__esModule", {
  configurable: true,
  value: true
});

@kftsehk
Copy link

kftsehk commented May 20, 2022

@RyanClementsHax I do not represent next.js, but imo next.js works at production, and to facilitate a particular kind of testing methodology isn't their primary objective.

@RyanClementsHax
Copy link
Contributor

@kftsehk I agree with you that I should adhere to the direction Next.js wants to move in with their internals rather than the other way around. The thing that concerns me is that this seems to break the contract of ESM in general thus jeopardizing anything that depends on Next.js to honor that contract.

@rodilo
Copy link

rodilo commented Oct 11, 2022

Where do you add this code? I've tried putting it in .storybook/preview.js but I get this error:

image

storybook is 6.4.9
next is 12.3.1
webpack 4

@qqharry21
Copy link

qqharry21 commented Oct 16, 2022

I add these codes down below .storybook/preview.js:

import * as nextImage from 'next/image';

Object.defineProperty(nextImage, 'default', {
  configurable: true,
  value: props => <img {...props} />,
});

and change the script of the package.json:

"storybook": "start-storybook -s ./public -p 6006",

It works for me to show NextImage in the Header component

<Image src='/vercel.svg' width={200} height={100} />

Hope it works for you guys

github-actions bot pushed a commit to open-sauced/app that referenced this issue Jan 17, 2023
github-actions bot pushed a commit to open-sauced/app that referenced this issue Jan 30, 2023
## [1.25.0](v1.24.0...v1.25.0) (2023-01-30)

### 🧑‍💻 Code Refactoring

* Renamed the ContributorTable to PullRequestTable ([#779](#779)) ([694d213](694d213)), closes [#681](#681)

### 🐛 Bug Fixes

* add fix for Storybook usage with Next 13 ([#792](#792)) ([bb10b2b](bb10b2b)), closes [/github.com/vercel/next.js/issues/36417#issuecomment-1117360509](https://github.com/open-sauced//github.com/vercel/next.js/issues/36417/issues/issuecomment-1117360509)
* correct repositories empty list message ([#778](#778)) ([18b7393](18b7393)), closes [#777](#777)
* make repositories table styles more responsive ([#773](#773)) ([fe5c6f5](fe5c6f5)), closes [#724](#724)
* mobile insights header layout break ([#795](#795)) ([0bc2f0b](0bc2f0b)), closes [#769](#769)
* remove usage of next/legacy/image ([#793](#793)) ([9264ffb](9264ffb))
* revert [#778](#778) ([f6e30e1](f6e30e1))
* use correct avatar URL for caching  ([#800](#800)) ([726f11b](726f11b)), closes [#757](#757) [#746](#746)

### 🍕 Features

* add `PullRequestSocialCard` component to design system ([#774](#774)) ([04600c2](04600c2)), closes [#716](#716)
* add `UserSettings` component to design system ([#788](#788)) ([dd9cabd](dd9cabd)), closes [#783](#783)
* set insight repo limit based on role ([#813](#813)) ([9e998f1](9e998f1))
* update to Next 13.1.x ([#758](#758)) ([72c2b64](72c2b64)), closes [#753](#753)
brandonroberts added a commit to open-sauced/app that referenced this issue Jan 30, 2023
* v1.25.0-beta.10 -> production (#815)

* chore(minor): release 1.25.0 [skip ci]

## [1.25.0](v1.24.0...v1.25.0) (2023-01-30)

### 🧑‍💻 Code Refactoring

* Renamed the ContributorTable to PullRequestTable ([#779](#779)) ([694d213](694d213)), closes [#681](#681)

### 🐛 Bug Fixes

* add fix for Storybook usage with Next 13 ([#792](#792)) ([bb10b2b](bb10b2b)), closes [/github.com/vercel/next.js/issues/36417#issuecomment-1117360509](https://github.com/open-sauced//github.com/vercel/next.js/issues/36417/issues/issuecomment-1117360509)
* correct repositories empty list message ([#778](#778)) ([18b7393](18b7393)), closes [#777](#777)
* make repositories table styles more responsive ([#773](#773)) ([fe5c6f5](fe5c6f5)), closes [#724](#724)
* mobile insights header layout break ([#795](#795)) ([0bc2f0b](0bc2f0b)), closes [#769](#769)
* remove usage of next/legacy/image ([#793](#793)) ([9264ffb](9264ffb))
* revert [#778](#778) ([f6e30e1](f6e30e1))
* use correct avatar URL for caching  ([#800](#800)) ([726f11b](726f11b)), closes [#757](#757) [#746](#746)

### 🍕 Features

* add `PullRequestSocialCard` component to design system ([#774](#774)) ([04600c2](04600c2)), closes [#716](#716)
* add `UserSettings` component to design system ([#788](#788)) ([dd9cabd](dd9cabd)), closes [#783](#783)
* set insight repo limit based on role ([#813](#813)) ([9e998f1](9e998f1))
* update to Next 13.1.x ([#758](#758)) ([72c2b64](72c2b64)), closes [#753](#753)
@leerob
Copy link
Member

leerob commented Jun 26, 2023

@leerob leerob closed this as completed Jun 26, 2023
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples locked
Projects
None yet
Development

No branches or pull requests

9 participants