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

Svelte: Native Svelte CSF #13772

Closed
wants to merge 5 commits into from
Closed

Conversation

j3rem1e
Copy link
Contributor

@j3rem1e j3rem1e commented Jan 29, 2021

Issue: #6653

What I did

A new implementation based on the discussion about a Svelte story format (@rixo and @BlackFenix2)

It supports:

  • args stories and stories without args ;
  • the "template" pattern for args stories, compatible with a svelte syntax ;
  • extractions of sources of the stories and compatible with addon-sources
  • decorators
  • knobs, actions, controls
  • storyshots (with a special jest transformation shipped under storybook/svelte/jest-transform)

How to test

  • Is this testable with Jest or Chromatic screenshots? Done
  • Does this need a new example in the kitchen sink apps? Done
  • Does this need an update to the documentation? Done

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Wow this is really cool!


return dedent`${code}
const { default: parser } = require('${parser}');
export const __dynamicStoriesGenerator = (m) => parser(m, ${JSON.stringify(storiesSources)});
Copy link
Member

@tmeasday tmeasday Jan 31, 2021

Choose a reason for hiding this comment

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

Can you explain what's happening here? It looks like it is generating the story list at runtime rather than at this point (build time). It would be (much) better if it was able to generate a story list statically!

Why does it need to do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to explain why I had to do this :)

Generating native Js stories from a svelte file can be hard:

<Story name="myStoryName" args={ <can be complex javascript expression > }/>

In this example, name is static, but any other properties can be complex expressions (for example by referencing common properties, etc). To generate a js story from this definition, I had to implement an ast->js generator and this can be hard to maintain and will depends on svelte internals.

So I choose to not follow this path and "execute" the stories in a special environment which will just collect every stories definition/properties, and disable components rendering. This is the parser function you see here.

At the end, I have a map of stories id to stories definition.

I can export them with module.exports = ...

However the svelte component is compiled as esm, so this syntax is not possible. I use it in the jest-transformer, because jest requires cjs module and in this context, svelte are compiled as cjs during tests. I don't want to require svelte files to be compiled to cjs because it will be a breaking change (and cjs is not really a step forward...)

I can't export them as named export because this map of stories is dynamic and named export should be statically defined.

A scenario I tried is introspecting the stories component to extract all stories name (I already do this to extract stories sources.), and for each stories, I can generate a named export. However, I can't introspect easily "imported stories" (story defined in another file and imported in another file). I'd like to support this feature because it allows to reuse stories in different contexts (there is a paragraph about this in the doc).

So I decided to use this "trick" : I export a special function which is used in loadCsf: if this function is defined, then loadCsf use the result instead of the module.

I knew I would have remarks on this change, it's the only change external to svelte - but I wanted to have your opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

A scenario I tried is introspecting the stories component to extract all stories name (I already do this to extract stories sources.), and for each stories, I can generate a named export. However, I can't introspect easily "imported stories" (story defined in another file and imported in another file). I'd like to support this feature because it allows to reuse stories in different contexts (there is a paragraph about this in the doc).

Can you expand on this part?

I think this is the only piece blocking doing everything in the loader and not modifying loadCsf, which we don't think is a good idea since it's important that CSF exports be statically analyzable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd have liked to be able to compose stories, like described somewhere in the documentation, but I can't find the page I though of. I will try to do without.

it's important that CSF exports be statically analyzable.

Can I ask why ? In the case of React, a story is a real react function, but In my case, nothing is reusable/importable.

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.

This is fantastic @j3rem1e 💯

I agree with @tmeasday that the dynamic exports deserve some discussion.

Also, unrelated but your PR made me think about it. WDYT about making component optional in the "raw" Svelte CSF, similar to what's been recentoly done here for angular: #8673

app/svelte/src/server/extract-sources.test.js Show resolved Hide resolved
docs/snippets/svelte/button-story-with-emojis.svelte.mdx Outdated Show resolved Hide resolved
@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 1, 2021

Also, unrelated but your PR made me think about it. WDYT about making component optional in the "raw" Svelte CSF, similar to what's been recentoly done here for angular: #8673

I have refactored in a local branch the rendering of Svelte stories (to improve decorators syntax and remove dependencies on Svelte internals - the code is much simpler now). I can add this change on this branch

@shilman shilman changed the title Native Svelte Story Format Svelte: Native Svelte CSF Feb 10, 2021
Copy link
Contributor

@phated phated 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 very cool! I am still a novice with Svelte, but this looks like a nice way to write stories. I had a few questions inline and I was also wonder if you updated this as per your last comment, @j3rem1e?

app/svelte/src/client/parse-stories.js Outdated Show resolved Hide resolved
examples/svelte-kitchen-sink/jest.config.js Outdated Show resolved Hide resolved
Comment on lines +21 to +24




Copy link
Contributor

Choose a reason for hiding this comment

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

Why so much whitespace in some of these snapshots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the same problem in 6.1 in the projects I manage. svelte keeps spaces in template, so snapshots generated by storyshoots has a lot of spaces. If you change just indentation in a svelte component, the snapshots failed.

Maybe I can try to implements a new snapshots serializer for svelte, but it will break every already generated snapshots

@phated
Copy link
Contributor

phated commented Feb 12, 2021

@j3rem1e I was chatting with some others on the team and it sounds like we want to incubate this concept a bit before we pull it into core. From my understanding, we can implement this as an addon, so people can opt-in to the feature. Once we are happy with everything, we can make the Svelte framework depend on the addon directly. I created a new repo: https://github.com/storybookjs/addon-svelte-csf - would you want to submit this work as a PR over there?

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 12, 2021

I'll try do that.

However, this PR needs some kind of "dynamic stories generation" for the reasons I gave above, I don't know yet how to implement this format without that.

@rcline
Copy link

rcline commented Feb 12, 2021

Very excited about this. Willing to help test the addon once ready!

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 13, 2021

I tried to statically generates stories, but without success.

I can easily extract stories name/id, which are required to be static, but every other parameters are too complex to be statically generated, without reimplementing an svelte ast -> js compiler, and I don't want to do that. It's too much work, with a hard dependencies an svelte internals.

A simple example :

<script>
  const defaultArgs = { status: true }
</script>

<Story name="myStory" args={...defaultArgs, name:'xx'}>
  ...
</Story>

I can generate :

const some_id = (args) => ({Component: ..., props:..});
some_id.args = .. // has to be generated from the ast 

This example is simple but in a real application, there will be function, import and others.

If someone has a workaround, I am interessed.

@shilman
Copy link
Member

shilman commented Feb 14, 2021

Is it possible to use the original source? I'm guessing the AST has line/column information attached, so you might be able to "copy-paste" pieces of the source to avoid having to write that aspect of code gen?

@shilman
Copy link
Member

shilman commented Feb 14, 2021

Alternatively, is there any way to get svelte to do the work for you?

export const NamedExport = someSvelteGeneratedIdentifier;

Where someSvelteGeneratedIdentifier is an object that matches the signature of a story export.

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 14, 2021

Is it possible to use the original source? I'm guessing the AST has line/column information attached, so you might be able to "copy-paste" pieces of the source to avoid having to write that aspect of code gen?

yes, the ast has a source/column, I use it for example to extract stories sources. However, in this case, it's not easy to use it because of dependencies on the <script/> tag.

In this example:

<script>
  const defaultArgs = { x:1 };

  let count = 0;
  function handleClick() {
   count++;
  }

  $: count2 = count * 2;
</script>

<Story name="myStory" args={...defaultArgs, y:2}>
  <div on:click={handleClick}>{count2}</div>
</Story> 

defaultArgs is defined in the script tag, so I have to "extract it". I can't copy/paste the script tag without analysing dependencies : i shouldn't extract countand handleClick, or worst the $: count2 = .. which is not valid js.

The script tag can also be defined in another language, like typescript or coffeescript.

Alternatively, is there any way to get svelte to do the work for you?

Exports can only be generated in a special<script context="module"></script> tag. this context is static and can't see the template/script. There is maybe a way to hack this, but I haven`t found it yet 😞

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 24, 2021

I have pushed here a first version of this PR into an addon: storybookjs/addon-svelte-csf#1

@shilman
Copy link
Member

shilman commented Feb 24, 2021

Awesome work @j3rem1e -- I'm closing this PR in favor of the new one 💯

@shilman shilman closed this Feb 24, 2021
@phated
Copy link
Contributor

phated commented Feb 24, 2021

@shilman we should close out the other 2 older ones as well

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.

5 participants