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 CSF Stories #1

Merged
merged 6 commits into from
Mar 6, 2021
Merged

Svelte CSF Stories #1

merged 6 commits into from
Mar 6, 2021

Conversation

j3rem1e
Copy link
Contributor

@j3rem1e j3rem1e commented Feb 24, 2021

Implementation of a svelte story format

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 tremendous @j3rem1e -- I'm thrilled you were able to get it working without modifying Storybook core 🙌

@Anthropic
Copy link

Anthropic commented Feb 25, 2021

@j3rem1e would it be possible, based on your knowledge gained from working on this epically awesome PR here, to actually include a storybook definition within a svelte component in future? Just curious, not even a suggestion.

I admit the documentation part of this PR had me initially thinking that would be possible, till I read it more carefully.

@j3rem1e you sir are a champion, can't wait to use this.

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 27, 2021

@Anthropic I don't think it will be easy to do that. maybe with a preprocessor, but mixin a component and his stories is probably not a good idea :

  • Story or Meta component should be included only in a Storybook, not in the final prod bundle
  • script tag should not be shared either, otherwise test code will be included in the prod bundle

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.

Had a few questions about how this works 😄

package.json Outdated Show resolved Hide resolved
src/parser/collect-stories.js Show resolved Hide resolved
src/parser/collect-stories.js Show resolved Hide resolved
src/parser/collect-stories.js Show resolved Hide resolved
src/parser/collect-stories.js Outdated Show resolved Hide resolved
src/parser/extract-id.test.js Outdated Show resolved Hide resolved
src/parser/extract-stories.ts Outdated Show resolved Hide resolved
src/parser/svelte-stories-loader.ts Show resolved Hide resolved
.map(([id]) => `export const ${id} = __storiesMetaData.stories[${JSON.stringify(id)}]`)
.join('\n');

const codeWithoutDefaultExport = code.replace('export default ', '//export default');
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In svelte, the component are exported as a default export. this is not valid in CSF, the default export should be the tag.

This line is just a 'cheap' way to transform the js generated by svelte by removing this default export.


return dedent`${codeWithoutDefaultExport}
const { default: parser } = require('${parser}');
const __storiesMetaData = parser(${componentName}, ${JSON.stringify(stories)});
Copy link
Contributor

Choose a reason for hiding this comment

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

So the parser is still run dynamically, even though the stories were already parsed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the Stories are executed in a context which disable rendering, but collect every args/loaders/parameters

@betaboon
Copy link

betaboon commented Mar 1, 2021

i just checked out this branch and ran npm run build followed by npm run storybook and ran into the following error for all svelte-stories:

WARNING in ./stories/button.stories.svelte
Module build failed (from ./dist/parser/svelte-stories-loader.js):
TypeError: name.replaceAll is not a function
    at extractId (/home/betaboon/src/js/addon-svelte-csf/dist/parser/extract-id.js:17:15)
    at Object.enter (/home/betaboon/src/js/addon-svelte-csf/dist/parser/extract-stories.js:86:43)
    at visit (/home/betaboon/src/js/addon-svelte-csf/node_modules/svelte/compiler.js:5256:11)
    at visit (/home/betaboon/src/js/addon-svelte-csf/node_modules/svelte/compiler.js:5288:13)
    at Object.walk (/home/betaboon/src/js/addon-svelte-csf/node_modules/svelte/compiler.js:5207:10)
    at extractStories (/home/betaboon/src/js/addon-svelte-csf/dist/parser/extract-stories.js:72:10)
    at Object.transformSvelteStories (/home/betaboon/src/js/addon-svelte-csf/dist/parser/svelte-stories-loader.js:64:52)

it is working for me with the following patch:

diff --git a/src/parser/extract-id.ts b/src/parser/extract-id.ts
index e47f87a..cb54669 100644
--- a/src/parser/extract-id.ts
+++ b/src/parser/extract-id.ts
@@ -4,5 +4,5 @@ export function extractId({ id, name }: { id?: string; name?: string }): string
     return id;
   }
 
-  return name.replaceAll(/[^a-zA-Z0-9_]/g, '_');
+  return name.replace(/[^a-zA-Z0-9_]/g, '_');
 }

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Mar 1, 2021

@betaboon this should be fixed, I removed replaceAll which is not supported by all platforms.

@betaboon
Copy link

betaboon commented Mar 1, 2021

@betaboon this should be fixed, I removed replaceAll which is not supported by all platforms.

i can confirm that it is fixed (after the force-push) :) thank you

@betaboon
Copy link

betaboon commented Mar 1, 2021

just fyi: using argTypes works great as well:

<script>
  import { Meta, Story, Template } from "@storybook/addon-svelte-csf";
  import Button from "./Button.svelte";

  const meta = {
    title: "Example/Button",
    component: Button,
    argTypes: {
      backgroundColor: { control: "color" },
      size: {
        control: {
          type: "inline-radio",
          options: ["small", "medium", "large"],
        },
      },
      onClick: { action: "onClick" },
    },
  };
</script>

<Meta {...meta} />

<Template let:args>
  <Button {...args} on:click={args.onClick} />
</Template>

<Story name="Primary" args={{ label: 'Button', primary: true }} />

<Story name="Secondary" args={{ label: 'Button' }} />

<Story name="Large" args={{ label: 'Button', size: 'large' }} />

<Story name="Small" args={{ label: 'Button', size: 'small' }} />

PS: would it be possible to get a pre-release on npm ? :)

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.

@shilman I'm inclined to say this is good-to-go unless you have any feedback on my couple of comments.

@shilman
Copy link
Member

shilman commented Mar 5, 2021

Sorry just seeing the notification now--will review in the morning 🙏

@shilman shilman changed the title Svelte Stories Svelte CSF Stories Mar 6, 2021
@shilman shilman merged commit f514912 into storybookjs:main Mar 6, 2021
@shilman shilman added enhancement New feature or request major Increment the major version when merged labels Mar 6, 2021
@@ -0,0 +1,25 @@
const svelte = require('svelte/compiler');

const parser = require.resolve('./parser/collect-stories').replace(/[/\\]/g, '/');
Copy link

Choose a reason for hiding this comment

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

Why [/\\] and not just [\\]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major Increment the major version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants