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

Add @storybook/nextjs framework #19382

Merged
merged 35 commits into from
Oct 21, 2022
Merged

Add @storybook/nextjs framework #19382

merged 35 commits into from
Oct 21, 2022

Conversation

kylegach
Copy link
Contributor

@kylegach kylegach commented Oct 6, 2022

What I did

Note: Because our current workflow requires this PR to be merged before we can consistently check it in CI, this work is "feature flagged" to avoid inadvertent use by users of sb@next init. See: #19478.

  • Add @storybook/nextjs framework
  • Add nextjs/default-js and nextjs/default-ts to repro configs
  • Add basic NEXTJS project type to CLI
    • Includes framework-specific template files
  • Update CLI logic
    • Add framework option to baseGenerator
    • Add hasFrameworkTemplates
      • Ensures frameworks with templates use those instead of templates for associated renderer
      • Follows pattern of hasInteractiveStories
    • Update copyComponents to copy common template files before (instead
      of after) framework/render template files
      • Allows framework/renderer template files to contain, e.g. Introduction.stories.mdx and have it not be overwritten
  • Apply STORYBOOK_REPRO_GENERATOR to env when running sb init in repro generator
    • Serves as a "feature flag" around detection of the framework in CLI

Note: This work is heavily based on storybook-addon-next by @RyanClementsHax and storybook-addon-next-router by @lifeiscontent. Thank you for your efforts!

How to test

  1. yarn generate-repros-next --template nextjs/default-js --local-registry
  2. yarn task --task sandbox --template nextjs/default-js --from-local-repro
  3. Repeat with nextjs/default-ts
  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@socket-security
Copy link

socket-security bot commented Oct 6, 2022

Socket Security Report

👍 No new dependency issues detected in pull request

Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ✅ no new install scripts
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ✅ no new native modules
Bin Script Confusion ✅ no new bin script confusions
Bin script shell injection ✅ no new bin script shell injection
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

code/frameworks/nextjs/README.md Show resolved Hide resolved
code/frameworks/nextjs/README.md Show resolved Hide resolved

- `nextConfigPath`: The absolute path to the `next.config.js`

### Next.js's Image Component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: One problem I've been having with my addon was that it wasn't very clear that more advanced configuration options weren't supported because of their reliance on either the resolved nextjs config (not just the file but the tons of defaults that nextjs adds which changes between versions btw), or a running nextjs server to optimize the images.

See this discussion (and probably take it over too) to discuss with the nextjs team about exposing a function to load and add defaults to the resolved nextjs config so we can support more features of images vercel/next.js#40891

code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
code/frameworks/nextjs/src/resolved-router-context.ts Outdated Show resolved Hide resolved
* // after main script path truncation
* scopedResolve('styled-jsx') === '/some/path/node_modules/styled-jsx'
*/
export const scopedResolve = (id: string): string => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If including this in the storybook code base, I'd recommend revisiting this function. This was a hack to get local development working where sample projects in the root repository's examples folder could reference the addon easily and not have packages conflict. This might be unneeded for storybook's monorepo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible alternative:

import { dirname } from 'path';
export function getBaseDir() {
return dirname(require.resolve('@storybook/cli/package.json'));
}

code/lib/cli/src/detect.test.ts Show resolved Hide resolved
code/frameworks/nextjs/README.md Show resolved Hide resolved
@RyanClementsHax
Copy link

question: what is the current position on supporting the vite builder? I've gotten a few issues on my addon's repo for vite support.

@kylegach
Copy link
Contributor Author

kylegach commented Oct 6, 2022

question: what is the current position on supporting the vite builder? I've gotten a few issues on my addon's repo for vite support.

Isn't Next Webpack-only? I don't understand why people would ask for that. I'm away from my computer at the moment; will look into those issues later for my own edification.

Thanks for taking a look, Ryan!

@RyanClementsHax
Copy link

question: what is the current position on supporting the vite builder? I've gotten a few issues on my addon's repo for vite support.

Isn't Next Webpack-only? I don't understand why people would ask for that. I'm away from my computer at the moment; will look into those issues later for my own edification.

Thanks for taking a look, Ryan!

Yup. It is. It seems there is a hunger to use the vite builder just for storybook though. I imagine this is possible too. I don’t know how much of the code can be shared however :/

@himyjan
Copy link

himyjan commented Oct 9, 2022

@storybook/react-vite builder work well with next.js
Storybook v7 example

@kylegach kylegach force-pushed the nextjs-framework-support branch from 4df9964 to 062a92e Compare October 12, 2022 15:43
kylegach added a commit that referenced this pull request Oct 12, 2022
Co-authored-by: Yann Braga <[email protected]>
@kylegach kylegach force-pushed the nextjs-framework-support branch 2 times, most recently from f887fa3 to 24f01bd Compare October 13, 2022 14:46
code/frameworks/nextjs/src/css/webpack.ts Show resolved Hide resolved
* https://webpack-3.cdn.bcebos.com/loaders/css-loader/#url
* https://webpack-3.cdn.bcebos.com/loaders/css-loader/#import
*/
const getImportAndUrlCssLoaderOptions = (nextConfig: NextConfig) =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its worth checking if with the upgrade of storybook to v7 we can get rid of css loader v4 support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Ryan, in Storybook 7 we only support Webpack5. Does that mean we can remove some of the code here? I'm a little lost in what actual chunks could be removed. Maybe you could help us navigate that? Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe.... it wasn't that some people were using webpack 4. My addon never supported webpack 4. It was just that some people because of npm versiom resolution were using various css loader major versions. I believe part of the problem was from some storybook internal dependencies but I can't remember.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RyanClementsHax/storybook-addon-next#22 here is one of the related issues

kylegach and others added 6 commits October 13, 2022 11:28
- Remove `frameworkToRenderer`
    - This information is already available from `baseGenerator`'s arguments
- Add `hasFrameworkTemplates`
    - Ensures frameworks with templates use those instead of templates for associated renderer
    - Follows pattern of `hasInteractiveStories`
- Update `copyComponents` to copy common template files before (instead
  of after) framework/render template files
    - Allows framework/renderer template files to contain, e.g. `Introduction.stories.mdx` and have it not be overwritten
- Apply STORYBOOK_REPRO_GENERATOR to env when running `sb init` in repro generator
@yannbf yannbf marked this pull request as ready for review October 20, 2022 21:20
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

@shilman shilman merged commit 4df3f6f into next Oct 21, 2022
@shilman shilman deleted the nextjs-framework-support branch October 21, 2022 14:37
@bertho-zero
Copy link
Contributor

It looks like there is no support for next/future/image (RyanClementsHax/storybook-addon-next#99)

Copy link
Member

shilman commented Oct 24, 2022

@kylegach @yannbf let's go through the open issues on the addon & see what we want to address in the framework for 7.0?

@RyanClementsHax
Copy link

@kylegach @yannbf let's go through the open issues on the addon & see what we want to address in the framework for 7.0?

I'd recommend doing this and also going through the closed issues. Some of the issues opened were valid but were automatically closed because no activity was recorded on them for some time.

@kylegach
Copy link
Contributor Author

It looks like there is no support for next/future/image (RyanClementsHax/storybook-addon-next#99)

@bertho-zero — It was decided to ship what we have ready (and what storybook-addon-next currently supports) and then add further features like that. We also need to coordinate with the Next.js folks about the best way to read the Next config. We definitely haven't forgotten about it!

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

Successfully merging this pull request may close these issues.

8 participants