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

CLI: Remove unsupported frameworks/renderers and improve builder detection #22492

Merged
merged 11 commits into from
May 11, 2023

Conversation

valentinpalkovic
Copy link
Contributor

Resolves #22349

What I did

  • I have removed broken/unsupported frameworks/renderers
  • Changed the detection of which particular builder should be used. Currently, we assume the vite builder as soon as a vite configuration can be resolved. Nowadays, a vite config is not mandatory for a vite app. The logic now looks for vite and webpack dependencies instead. If just one dependency can be found, it will be taken as builder. If both builder can be resolved, the user will be asked in a prompt which builder to use.

How to test

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

@valentinpalkovic valentinpalkovic added the ci:daily Run the CI jobs that normally run in the daily job. label May 10, 2023
@valentinpalkovic valentinpalkovic marked this pull request as ready for review May 10, 2023 13:19
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

This looks like a good change to me!

code/lib/cli/src/detect.ts Outdated Show resolved Hide resolved
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 LGTM except for the comment above

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.

This looks awesome!

There's one side effect of this PR which I think @storybookjs/core and @kylegach should weigh in on.

Back in Storybook 6.2.9 days, we moved many frameworks out of the monorepo: rax, riot, marko, aurelia, marionette and mithrill

The thing is, there is documentation for those in tutorials, blog posts or READMEs in those (now moved) repos, that tell people to run stuff like npx sb init --type marko --builder webpack5. When @latest was Storybook 6, that made sense, but not anymore.

Users (not a huge amount, but some) are trying to install Storybook in some of these projects by following their instructions, and with Storybook 7 they get an error like so:
Error: Could not find the framework (@storybook/marko-webpack5) or renderer (@storybook/marko) package

That makes sense, because these packages were not updated to the new frameworks format we have. This PR solves that by cleaning up code related to these now unsupported frameworks and removing them from the --type option. As a result, users will get another error message instead:


The project types currently supported by Storybook are: (and then a list of what's available here)

Even though that’s better, users might still be a little confused or frustrated, as they are following docs from those official READMEs and end up in a message that says it's unsupported.

Now, it's probably not in the scope of this task, but should be a followup at least, but I wonder what we should do regarding this situation:

  1. We can go and find and update these unsupported framework READMEs to use npx storybook@6 ... instead.
  2. We can alert the maintainers of each of these packages, though I think they're all currently unmaintained
  3. We can add a "looking for maintainers" banner in each of these and mention that these frameworks only work for Storybook 6
  4. We can add a check for the type flag wheen users run storybook --type <one-of-the-unsupported-frameworks> and explicitly mention that they are not supported in Storybook 7 and users should file an issue in the framework's repo if they're looking for Storybook 7 support
  5. We can do nothing, and just let users decide for themselves what to do

There's also another unfortunate situation, but not related to this PR (though I only noticed because of this PR):
Even if we updated the docs of unsupported frameworks and told people to use storybook@6 init instead, given that the CLI itself installs latest versions of the packages, users end up with the following package.json dependencies with a mix of Storybook 6 and 7 packages:

"devDependencies": {
    "@babel/core": "^7.21.8",
    "@storybook/addon-actions": "^7.0.10",
    "@storybook/addon-essentials": "^7.0.10",
    "@storybook/addon-links": "^7.0.10",
    "@storybook/builder-webpack5": "^7.0.10",
    "@storybook/manager-webpack5": "^6.5.16",
    "@storybook/marionette": "^6.2.9",
    "babel-loader": "^8.3.0"
}

This will obviously break. Seems like users would need to get a manual installation step, or some guidance to get these Storybooks in a working state. I honestly don't know how much effort we should put into the scenario for the unsupported frameworks, especially if there's little to no movement on the user base and maintainer base, but I thought I'd bring this to the team, because we still end up getting crash reports from users reaching any of those scenarios.

@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 11, 2023
@shilman
Copy link
Member

shilman commented May 11, 2023

That all sounds good but we should probably just deprecate all of them except for Marko which has a maintainer

@yannbf yannbf changed the title Remove unsupported/broken frameworks/renderers + Handle vite detection properly CLI: Remove unsupported frameworks/renderers and improve builder detection May 11, 2023
@yannbf yannbf added the cli label May 11, 2023
@valentinpalkovic
Copy link
Contributor Author

@shilman What do you mean by "deprecating" them? Should I revert my first commit and process.exit with a message as soon as rax, riot, aurelia, marionette or mithrill are used? What is the action item here?

@shilman
Copy link
Member

shilman commented May 11, 2023

@valentinpalkovic No the PR is good. My comment is in response to @yannbf 's suggestions, which sounds like a lot of work. Removing them from the CLI is great and we should also remove them from the storybook org. I'll take care of that

@IanVS
Copy link
Member

IanVS commented May 11, 2023

Does this replace #22051 ?

@valentinpalkovic valentinpalkovic merged commit d5cc922 into next May 11, 2023
@valentinpalkovic valentinpalkovic deleted the valentin/fix-could-not-find-framework-renderer branch May 11, 2023 12:54
@yannbf yannbf mentioned this pull request May 11, 2023
5 tasks
@kylegach
Copy link
Contributor

Even if we updated the docs of unsupported frameworks and told people to use storybook@6 init instead, given that the CLI itself installs latest versions of the packages, users end up with the following package.json dependencies with a mix of Storybook 6 and 7 packages:

Until we fix this issue, I agree that the best course of action is the current behavior of displaying the supported list of frameworks when trying to init with an unsupported type.

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 12, 2023
shilman pushed a commit that referenced this pull request May 12, 2023
…d-framework-renderer

CLI: Remove unsupported frameworks/renderers and improve builder detection
@shilman shilman mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storybook init: Could not find the framework (abc) or renderer (xyz) package
5 participants