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: Add Solid integration #20991

Merged
merged 6 commits into from
Feb 10, 2023
Merged

CLI: Add Solid integration #20991

merged 6 commits into from
Feb 10, 2023

Conversation

webblocksapp
Copy link
Contributor

What I did

This feature adds the Storybook SolidJS framework to the Storybook CLI (Only client-side rendering). It was registered as an external framework while it becomes more popular to be integrated as part of the Storybook core frameworks.

This PR also adds support for reading external frameworks with frameworks and renderers folder structure, where each one is an npm package (Current standard used by core frameworks).

-frameworks
  |-solid-vite
-renderers
  |-solid

Previous implementations for external frameworks like qwik uses a single folder for loading the framework and renderer being only a single npm package. You can see the example here.

It's pending to define if both standards will be used for external frameworks - @JReinhold, @tmeasday.

The Storybook SolidJS framework is on a beta version, some features are still under development however the main core functionalities are working for visualizing stories using SolidJS and fine-grained updates.

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

@shilman shilman changed the title Solid cli integration CLI: Add Solid integration Feb 7, 2023
@JReinhold JReinhold added the ci:daily Run the CI jobs that normally run in the daily job. label Feb 9, 2023
@JReinhold JReinhold self-assigned this Feb 9, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

💪 This is really good!
I've tested:

  1. ✅ It works in a fresh SolidJS project with sb init
  2. ✅ The sandbox creation works.
  3. daily CI workflow still passes
  4. ❌ Initializing a Qwik project would fail because it tried to install a renderer that was undefined. I've pushed a fix for it that makes it work, hope you don't mind.

Everything checks out.

The logic around external frameworks is getting a bit convoluted, but I think that's okay since all of this should only be temporary until we move external frameworks out of the CLI altogether.

@@ -45,15 +45,32 @@ const getBuilderDetails = (builder: string) => {

const getExternalFramework = (framework: string) =>
externalFrameworks.find(
(exFramework) => exFramework.name === framework || exFramework.packageName === framework
(exFramework) =>
framework !== undefined &&
Copy link
Contributor

Choose a reason for hiding this comment

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

why would framework be undefined here, parameter typings suggests it can't be?

I'll admit though that I'm very confused about these typings, some places it's optional, other places it's not.

Copy link
Contributor Author

@webblocksapp webblocksapp Feb 9, 2023

Choose a reason for hiding this comment

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

The logic around external frameworks is getting a bit convoluted, but I think that's okay since all of this should only be temporary until we move external frameworks out of the CLI altogether.

I agree. It needs some refactoring in the future once you have standardized the new way for handling frameworks.

why would framework be undefined here, parameter typings suggests it can't be?

I think it's because the' undefined' type is considered in ts a subtype of all other types, including string.
For this reason, ts doesn't complain at line 92 when receives framework? from getFrameworkDetails fn. So It's needed to add this possible scenario when framework is undefined.

@JReinhold JReinhold merged commit 4fa98c1 into storybookjs:next Feb 10, 2023
@jonniebigodes
Copy link
Contributor

@webblocksapp this is fantastic. Thank you so much for taking the time and effort to introduce support for SolidJS. We appreciate it 🙏 ! I want to follow up on one thing that was left out of this pull request. If you're ok with it. Can you message me on our Discord Server (same username) so that we can go over it?

Looking forward to hearing from you.

Hope you have a fantastic weekend.

Stay safe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. cli feature request solid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants