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

Core: Improve framework field validation #21299

Merged
merged 1 commit into from
Feb 28, 2023
Merged

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Feb 28, 2023

Closes #20651

What I did

The framework field is not validated in three different steps:

  1. Is there no framework field? -> throw error, tell user to run automigrate
  2. Is it a renderer package? -> throw error, tell user to run automigrate
  3. Is it a known framework package? -> all good
  4. Is it neither of the above? Does the package at least get resolved? else throw error

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Change the framework field to:
    scenario 1: remove framework field
    scenario 2: framework field value: @storybook/react-vite
    scenario 3: framework field value: react
    scenario 4: framework field value: foo

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"]

@yannbf yannbf requested a review from shilman February 28, 2023 14:48
@yannbf yannbf force-pushed the fix/validate-framework-field branch 2 times, most recently from cc8123f to 5f44f0a Compare February 28, 2023 15:58
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.

Generally looks good. Have a couple suggested improvements

code/lib/core-common/src/utils/validate-config.ts Outdated Show resolved Hide resolved
code/lib/core-server/src/build-dev.ts Show resolved Hide resolved
@yannbf yannbf force-pushed the fix/validate-framework-field branch from 5f44f0a to 5dcfe56 Compare February 28, 2023 17:03
@yannbf yannbf requested a review from shilman February 28, 2023 17:06
@shilman shilman merged commit d2d6f9d into next Feb 28, 2023
@shilman shilman deleted the fix/validate-framework-field branch February 28, 2023 17:36
https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#new-framework-api\n`;

// Throw error if there is no framework field
// TODO: maybe this error should not be thrown if we allow empty Storybooks that only use "refs" for composition
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this is not something we allow/support.
The UI will show you a "empty storybook" warning if you'd do that.

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.

Ensure users have a main.js framework field as part of the boot-up experience
3 participants