SolidJS framework feedback #20584

webblocksapp · 2023-01-11T19:53:06Z

Not for merging

This PR is the initial work on a SolidJS framework. As agreed with @JReinhold it is not meant to be merged, it is meant to collect feedback and suggestions. Once that has been done, this PR will be closed and the work will be solidified in a new repo.

JReinhold · 2023-01-13T21:02:38Z

Marked this as "Ready for Review" just to trigger full CI suite.

JReinhold

I did a thorough review, it looks really good!

One thing that I think should be fixed though, is that it doesn't work if stories don't have a render property. For some reason it doesn't pick up your exported render function from render.tsx, and I'm unsure how that is supposed to work exactly.

Maybe @tmeasday has some guidance about that?

JReinhold · 2023-01-13T22:39:50Z

code/lib/cli/src/automigrate/fixes/new-frameworks.ts

+  '@storybook/solid': {   
+    vite: '@storybook/solid',
+  },


You can remove this, as no one will be migrating from an old solid setup because that doesn't exist.

JReinhold · 2023-01-13T22:44:31Z

code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts

+      applyLoaders,
+      unboundStoryFn,
+      playFunction,
+      prepareContext: prepareStoryContext,


there's no reason to rename this to prepareStoryContext, just keep prepareContext

JReinhold · 2023-01-13T22:47:27Z

code/lib/preview-api/src/modules/store/csf/prepareStory.ts

+  // This function is invoked at StoryRender.ts, with this
+  // the context is prepared before invoking the render function.


Suggested change

// This function is invoked at StoryRender.ts, with this

// the context is prepared before invoking the render function.

// prepareContext is invoked at StoryRender.render()

// the context is prepared before invoking the render function, instead of here directly

// to ensure args don't loose there special properties set by the renderer

// eg. reactive proxies set by frameworks like SolidJS or Vue

JReinhold · 2023-01-13T22:50:12Z

code/renderers/solid/src/config.ts

@@ -0,0 +1,7 @@
+import { parameters as docsParams } from './docs/config';
+
+export const parameters = { framework: 'solid', ...docsParams };


Based on the other renderers

Suggested change

export const parameters = { framework: 'solid', ...docsParams };

export const parameters = { framework: 'solid' as const, ...docsParams };

JReinhold · 2023-01-13T23:20:24Z

code/renderers/solid/src/render.tsx

+/**
+ * Checks when the story requires to be remounted.
+ * Elements outside the story requires a whole re-render.
+ * e.g. dark theme, show grid, etc...
+ */
+const remount = (force: boolean, context: StoryContext<SolidRenderer>) => {
+  let flag = false;
+
+  if (force) {
+    flag = true;
+  } else if (!Object.is(globals, context.globals)) {
+    // Globals refers to storybook visualization options.
+    globals = context.globals;
+    flag = true;
+  }
+  return flag;
+};


Is the point here to force a re-mount when globals change?

Nit: I think the logic can be simplified a little bit.

Suggested change

/**

* Checks when the story requires to be remounted.

* Elements outside the story requires a whole re-render.

* e.g. dark theme, show grid, etc...

*/

const remount = (force: boolean, context: StoryContext<SolidRenderer>) => {

let flag = false;

if (force) {

flag = true;

} else if (!Object.is(globals, context.globals)) {

// Globals refers to storybook visualization options.

globals = context.globals;

flag = true;

}

return flag;

};

/**

* Checks when the story requires to be remounted.

* Elements outside the story requires a whole re-render.

* e.g. dark theme, show grid, etc...

*/

const shouldRemount = (force: boolean, context: StoryContext<SolidRenderer>) => {

if (force) {

return true;

}

if (!Object.is(globals, context.globals)) {

// Globals refers to storybook visualization options.

globals = context.globals;

return true;

}

return false;

};

Yes because fine-grained updates are not possible in React, so it needs to re-run the component to update the storybook canvas with globals. In globals doesn't matter if the component re-renders because they don't have a direct relationship with it.

I don't think this is the "correct" behaviour. Globals should cause a re-render in the same way that args do.

JReinhold · 2023-01-13T23:27:18Z

code/renderers/solid/src/render.tsx

+   * If remount is required, the whole root node is re-rendered.
+   */
+  if (remount(forceRemount, storyContext)) {
+    disposeStory?.();


The logic here and the singleton store assumes that only one story will be rendered at once. But in docs mode that is not the case because there we render multiple stories at the same time.
In this case that will mean that they all read and write from the same singleton store above - conflicting - and on this line they'll dispose each other immediately.
You can reproduce this by adding multiple stories and checking the autodocs page, it will only show the last story of the page because the others will have been disposed.

You'd probably need a map of stories being rendered similar to what we have in the Svelte renderer. https://github.com/storybookjs/storybook/blob/next/code/renderers/svelte/src/render.ts#L14
You can key the map by canvasElement which will be unique, and I think the value would then be a combination of a store and a dispose function.

Let me know if you need further assistance with this.

You can of course postpone this change, and say that the initial version doesn't support docs mode.

Thanks for this clarification, this is something that I have pending after having fine-grained updates working.

JReinhold · 2023-01-13T23:33:09Z

code/renderers/solid/template/cli/js/Button.stories.jsx

+
+// More on writing stories with args: https://storybook.js.org/docs/7.0/html/writing-stories/args
+export const Primary = {
+  args: {},


There should probably be some content on the button to not confuse people.

Suggested change

args: {},

args: { children: 'Click Me' },

Right, I started with an elementary template for simplifying my workflow. But then I was planning to re-create again the default example templates that have the other frameworks (using React as a reference)

JReinhold · 2023-01-13T23:33:35Z

code/renderers/solid/template/cli/ts/Button.stories.tsx

+
+// More on writing stories with args: https://storybook.js.org/docs/7.0/html/writing-stories/args
+export const Primary: Story = {
+  args: {},


Suggested change

args: {},

args: { children: 'Click Me' },

I agree, the same as the previous comment.

JReinhold · 2023-01-13T23:34:47Z

code/renderers/solid/template/stories/html-mdx.stories.mdx

@@ -0,0 +1,21 @@
+import { Meta, Story, Canvas } from '@storybook/addon-docs';


As stories defined in MDX is being deprecated in 7.0 I suggest you remove this template file.

JReinhold · 2023-01-13T23:56:12Z

Changes to `prepareStory`

tl;dr
The author has needed to make some changes to how we prepare stories, in order to make args reactive in SolidJS - ie. react to Controls changes. I think it looks good but I'd like someone more experienced like @tmeasday to check too if there's any problems with the proposal.

I've gone back and forth with @webblocksapp on Discord about this, and the way Storybook currently prepare stories means that reactive args gets lost in SolidJS. That's because for Solid to react to args/props changes it converts them all to Proxies behind the scenes, and when we do the argType mapping in prepareStory the Proxies are destroyed and args becomes a plain object again.

The proposal here is to change the flow so that the context is prepared after being passed to renderToCanvas so that the proxies stay intact. I've tested that argType mapping still works in React, but we should probably test other frameworks like Vue and Angular too though.

There's a chance that this change will also make reactive args in Vue3 possible, something that we currently can't do without re-creating the whole Vue story.

Splitting up the PR

@webblocksapp we've made som decisions on how we plan on supporting community frameworks like this in the future. We're going to make this monorepo only contain Core frameworks and move the rest out to other separate repos. This means that we also won't have SolidJS in the monorepo.

Long-term (we don't know when yet, but after 7.0 release at least) we'll make it possible for users to run something like npx sb init --framework storyook-solidjs-vite that will point to an npm package that then installs the necessary thing.

Short-term we'll accept PRs that changes the CLI here so that we can detect SolidJS projects and install your framework when we detect it.

So my suggestion here would be to close this PR when you want to, and split it up into three things:

A separate repo that contains two packages from here: storybook-solidjs-vite and storybook-solidjs (looks like storybook-solid is taken)
A PR that contains your changes with the prepareContext call.
A PR that contains your changes to the CLI so it detects SolidJS and installs storybook-solidjs-vite

For the separate repo, it's up to you if you want to create it yourself, or if we should create a repo under this storybookjs org, and you can continue the work there.

tmeasday · 2023-01-15T10:38:09Z

One thing that I think should be fixed though, is that it doesn't work if stories don't have a render property. For some reason it doesn't pick up your exported render function from render.tsx, and I'm unsure how that is supposed to work exactly.

Maybe @tmeasday has some guidance about that?

It should be picking it up from the /preview.js file, which re-exports src/config.ts (<- PS I'd call that src/preview.ts, "config" is the old name, we just have not updated other renderers).

The author has needed to make some changes to how we prepare stories, in order to make args reactive in SolidJS - ie. react to Controls changes. I think it looks good but I'd like someone more experienced like @tmeasday to check too if there's any problems with the proposal.

I think I need some more context about this, shall we discuss @JReinhold?

webblocksapp · 2023-01-16T14:55:18Z

Thanks @JReinhold for the deep review, I will start working today with your code suggestions.

Related to this:

One thing that I think should be fixed though, is that it doesn't work if stories don't have a render property. For some reason it doesn't pick up your exported render function from render.tsx, and I'm unsure how that is supposed to work exactly.

I have this story example on my local env. and is working without problems. Could you please provide your failing story to take a look?

export const Secondary = {
  args: {
    disabled: true,
    children: "Hello World",
  },
};

Related to splitting up the PR:

I would like to have the needed separate repos under your storybookjs org, I think we can save work re-migrating again a separate repo from my own. Also I prefer those packages to be under your organization so they will have more visibility, @storybook/solid, @storybook/solid-vite.
I will proceed with that once you have confirmed to apply this solution.
Got it.

So I'm pending for 1 and 2 bullets to be confirmed for continuing the development of the solidjs storybook extension.

tmeasday · 2023-01-16T21:30:29Z

We discussed the preview changes, where arg type mapping is pulled "up" the rendering chain. The concern was that it is a breaking change in that now decorators will receive mapped/filtered args, whereas in the past they have not.

It seems like that's better behaviour anyway. After surveying a little internally we've decided we are OK with pushing forward with that change. One thing I'd like to see if we are making that change is some unit tests demonstrating the mapping applying to decorators.

JReinhold · 2023-01-17T10:36:38Z

One thing that I think should be fixed though, is that it doesn't work if stories don't have a render property. For some reason it doesn't pick up your exported render function from render.tsx, and I'm unsure how that is supposed to work exactly.

I have this story example on my local env. and is working without problems. Could you please provide your failing story to take a look?

Sorry about that, I interpreted an error wrong. Everything is working as expected, it was just a brainfart on my end.

I would like to have the needed separate repos under your storybookjs org, I think we can save work re-migrating again a separate repo from my own. Also I prefer those packages to be under your organization so they will have more visibility, @storybook/solid, @storybook/solid-vite.

I've created a repo here and given you admin access: https://github.com/storybookjs/solidjs . When I get time I'll add some basic tooling if you want me to. It'll be a monorepo for anything solid related, as the Solid renderer and Solid-Vite framework will probably be developed together.
We won't be releasing them under the @storybook npm namespace though for security reasons, but I think storybook-solidjs and storybook-solidjs-vite is also fine.

The first step towards making the necessary changes to core will be to make a separate PR for them, so we can see tests running. And as Tom said we also want a few unit tests, something that I can probably help out with if you need that.

webblocksapp · 2023-01-19T00:54:38Z

Thanks @JReinhold and @tmeasday for your feedback.
I will start working with the core change as first step for having args mapped before the render fn.
I hope to have the PR maybe at the end of this week or starting next.

I've accepted too the repo invitation, so I will start the solid framework migration as soon I have PR the core change.

webblocksapp · 2023-01-23T22:19:58Z

Hi @JReinhold and @tmeasday, I've just PRed the core changes. I'm not able to label the PR, so I added it inside the description as a Breaking Change.

I modified 2 unit test files however they are failing due to snapshots. Locally I ran both tests with the -u flag, they are passing however the console threw me 0 snapshots for each file.

I attach PR #20755 and I continue aware of your comments for moving forward with points 1 and 3 or if the current PR needs more changes.

A separate repo that contains two packages from here: storybook-solidjs-vite and storybook-solidjs (looks like storybook-solid is taken)
~~A PR that contains your changes with the prepareContext call.~~
A PR that contains your changes to the CLI so it detects SolidJS and installs storybook-solidjs-vite

webblocksapp added 14 commits December 31, 2022 13:54

Initial SolidJS framework scaffolding

4f92c5d

Updated preset with vite-plugin-solid

d6055f7

Added solid renderer boilerplate

1c7fa8e

Added solid as repro-template

5b71882

Registered new generator for Solid

f208284

Fixed solid storybook auto migration error

cfc1551

Configured solid sandbox for further development

f9ad586

Initial Button component as SolidJS template

ce9113a

Solid renderer definition

8b64fbd

Working Solid render initial prototype.

e157e03

Changed store structure for story context.

2ccf69d

Added fine grained updates engine.

57d8f45

solid renderer tsup config for dev. WIP

b1ed4a0

Merge remote-tracking branch 'upstream/next' into solid-js-framework

17a701e

JReinhold changed the title ~~Solid js framework~~ SolidJS framework feedback Jan 11, 2023

webblocksapp added 4 commits January 11, 2023 20:15

Clean api definition for intercepting story props.

01c6384

Refactored reactivity with a decorator. Removed args mapper.

80d025f

Updated minor patch versions.

bf6dc7f

Refactored story context mapping.

ff32212

JReinhold marked this pull request as ready for review January 13, 2023 21:02

JReinhold self-assigned this Jan 13, 2023

JReinhold added the ci:merged Run the CI jobs that normally run when merged. label Jan 13, 2023

webblocksapp added 2 commits January 13, 2023 16:12

Added remount logic for globals at render.tsx

8642a22

Merge remote-tracking branch 'upstream/next' into solid-js-framework

90ad0f1

JReinhold reviewed Jan 13, 2023

View reviewed changes

ndelangen assigned ndelangen and unassigned JReinhold Jan 17, 2023

webblocksapp mentioned this pull request Jan 23, 2023

Core: Story context is prepared before for supporting fine grained updates #20755

Merged

5 tasks

JReinhold closed this Jan 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SolidJS framework feedback #20584

SolidJS framework feedback #20584

webblocksapp commented Jan 11, 2023 •

edited by JReinhold

Loading

JReinhold commented Jan 13, 2023

JReinhold left a comment

JReinhold Jan 13, 2023

JReinhold Jan 13, 2023

JReinhold Jan 13, 2023

JReinhold Jan 13, 2023

JReinhold Jan 13, 2023

webblocksapp Jan 16, 2023 •

edited

Loading

tmeasday Jan 16, 2023

JReinhold Jan 13, 2023

webblocksapp Jan 16, 2023

JReinhold Jan 13, 2023

webblocksapp Jan 16, 2023

JReinhold Jan 13, 2023

webblocksapp Jan 16, 2023

JReinhold Jan 13, 2023

JReinhold commented Jan 13, 2023

tmeasday commented Jan 15, 2023 •

edited

Loading

webblocksapp commented Jan 16, 2023 •

edited

Loading

tmeasday commented Jan 16, 2023 •

edited

Loading

JReinhold commented Jan 17, 2023 •

edited

Loading

webblocksapp commented Jan 19, 2023

webblocksapp commented Jan 23, 2023

		// This function is invoked at StoryRender.ts, with this
		// the context is prepared before invoking the render function.

-  // This function is invoked at StoryRender.ts, with this
-  // the context is prepared before invoking the render function.
+  // prepareContext is invoked at StoryRender.render()
+  // the context is prepared before invoking the render function, instead of here directly
+  // to ensure args don't loose there special properties set by the renderer
+  // eg. reactive proxies set by frameworks like SolidJS or Vue

		@@ -0,0 +1,7 @@
		import { parameters as docsParams } from './docs/config';

		export const parameters = { framework: 'solid', ...docsParams };

		@@ -0,0 +1,21 @@
		import { Meta, Story, Canvas } from '@storybook/addon-docs';

SolidJS framework feedback #20584

SolidJS framework feedback #20584

Conversation

webblocksapp commented Jan 11, 2023 • edited by JReinhold Loading

Not for merging

JReinhold commented Jan 13, 2023

JReinhold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webblocksapp Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JReinhold commented Jan 13, 2023

Changes to prepareStory

Splitting up the PR

tmeasday commented Jan 15, 2023 • edited Loading

webblocksapp commented Jan 16, 2023 • edited Loading

tmeasday commented Jan 16, 2023 • edited Loading

JReinhold commented Jan 17, 2023 • edited Loading

webblocksapp commented Jan 19, 2023

webblocksapp commented Jan 23, 2023

webblocksapp commented Jan 11, 2023 •

edited by JReinhold

Loading

webblocksapp Jan 16, 2023 •

edited

Loading

Changes to `prepareStory`

tmeasday commented Jan 15, 2023 •

edited

Loading

webblocksapp commented Jan 16, 2023 •

edited

Loading

tmeasday commented Jan 16, 2023 •

edited

Loading

JReinhold commented Jan 17, 2023 •

edited

Loading