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 preview type and update CLI template #21205

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Feb 22, 2023

Closes #20950

What I did

  • export Preview from all renderers
  • Update CLI templates

I didn't update the documentation yet, or added a runtime warning for old style preview.js files. Maybe we can outscope that for now?

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Check the generated preview!
    -->

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@@ -193,21 +208,27 @@ describe('configurePreview', () => {

expect(previewConfigPath).toEqual('./.storybook/preview.ts');
expect(previewConfigContent).toMatchInlineSnapshot(`
"import { setCompodocJson } from \\"@storybook/addon-docs/angular\\";
"import type { Preview } from '@storybook/angular'
Copy link
Member

Choose a reason for hiding this comment

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

I think the end result should be like the other import?

import type { Preview } from \\"@storybook/angular\\"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, maybe we should run prettier on it, using user config, if possible. The main.js template you wrote is actually using single quotes everywhere:

  if (isTypescript) {
    imports.push(`import type { StorybookConfig } from '${frameworkPackage}';`);
  } else {
    imports.push(`/** @type { import('${frameworkPackage}').StorybookConfig } */`);
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettified the files based on user config

@@ -114,20 +115,34 @@ export async function configurePreview(options: ConfigurePreviewOptions) {
return;
}

const prefix = [
isTypescript ? `import type { Preview } from '@storybook/${options.rendererId}'` : '',
Copy link
Member

@yannbf yannbf Feb 22, 2023

Choose a reason for hiding this comment

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

I wonder how this is going to work with community frameworks/renderers, it's better if the entire package comes as a parameter, rather than half of the import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure as well. Do we have an community framework example that I could test?
@JReinhold Do you know more about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@yannbf
Copy link
Member

yannbf commented Feb 22, 2023

I'm quite curious regarding the types coming from renderers. Checking this PR it got me thinking that React users would have the same types experience if they're using @storybook/react-vite or @storybook/react-webpack, however they probably want different types if they're using @storybook/nextjs, given that Nextjs provides out of the box routing and other features that can be configured at the parameter level. Does this make sense? Or should we just do some sort of augmentation in the future for scenarios like this?

@kasperpeulen
Copy link
Contributor Author

Does this make sense? Or should we just do some sort of augmentation in the future for scenarios like this?

@yannbf I think module augmentation would suffice:

declare module '@storybook/react' {
  interface Parameters
    extends StorybookParameters,
      ChromaticParameters,
      DocsParameters,
      NextjsParameters {}
}

I like that the csf files only import types from preview, and are kind of decoupled from the builder.

code/lib/cli/package.json Outdated Show resolved Hide resolved
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Awesome stuff!!

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.

Migration: Preview default exports
3 participants