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

[Feature Request]: Improve how automigrations read main.js #20955

Closed
yannbf opened this issue Feb 6, 2023 · 3 comments · Fixed by #20647 or #21168
Closed

[Feature Request]: Improve how automigrations read main.js #20955

yannbf opened this issue Feb 6, 2023 · 3 comments · Fixed by #20647 or #21168

Comments

@yannbf
Copy link
Member

yannbf commented Feb 6, 2023

Is your feature request related to a problem? Please describe

The Storybook automigrations follow this pattern:

  1. check (do assertions to know whether the automigration is needed)
  2. prompt (ask user if they want migration to be done)
  3. run (perform a migration on user's behalf)

Currently, we use AST based tooling (csf-tools) to both read main.js values in the check step, and also to manipulate main.js in the run step.

The problem

The way we read main.js by using AST analysis is error-prone given that users might have an unconventional format of their main.js files, e.g.

import baseConfig from '../storybook-utils/base-config'
export default baseConfig
import baseConfig from '../storybook-utils/base-config'
export default {
   ...baseConfig,
   stories: []
}
import baseConfig, { mergeConfig } from '../storybook-utils/base-config'
export default mergeConfig(baseConfig, {
   ...baseConfig,
   stories: []
})

Here's an example of an issue caused by it: #20940 (more users got affected by it)

Describe the solution you'd like

The automigrations should be improved in two ways:

Evaluating main.js with presets.apply in the run step

This way, it doesn't really matter what the main.js looks like. We will always be able to evaluate what we need. This comes with a small performance cost, and possibility of having some issues e.g. crashing if automigrations run in a project that has not installed dependencies.

Adding warnings in the check step

If users have a non AST friendly main.js, we should make sure that the automigrations don't crash, but rather tell the users that it was not possible to be done because their main.js is unconventional and we recommend changing, and provide info for them to do the migration manually themselves

@shilman
Copy link
Member

shilman commented Feb 7, 2023

Related from #20921

Currently getStorybookInfo incorrectly returns a renderer as a framework, which is wrong and may cause bugs in various uses

  • Remove framework/frameworkPackage from getStorybookInfo
  • Get rid of the 6.5 frameworks migration
    • Update the 7.0 new-frameworks migration to:
    • Deal with an empty framework field since the old migration won't run
    • Deal with an old framework field as it already does
    • Run early in the automigrations list so that later automigrations have access to framework

@shilman
Copy link
Member

shilman commented Feb 20, 2023

Yowza!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.51 containing PR #20647 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

@shilman
Copy link
Member

shilman commented Feb 21, 2023

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.53 containing PR #21168 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

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