tobiasdiez / storybook-vue-addon

Storybook stories in native Vue format
MIT License
46 stars 1 forks source link

fix: gracefully handle stories named the same as standard javascript keywords #41

Closed tobiasdiez closed 1 year ago

tobiasdiez commented 1 year ago

Having a story with the name 'Default' leads to the error:

18:39:56 [vite] Internal server error: Unexpected token (33:5)
  31 | }
  32 |     export const default = () => Object.assign({render: renderdefault}, _sfc_main)
> 33 |     default.storyName = 'Default'
     |     ^
  34 |     default.parameters = {
  35 |       docs: { source: { code: `<n-button>Button</n-button>` } },
  36 |     };
  Plugin: storybook-vue-addon

This is fixed by changing the story export to be capitalized. Fixes #48.

tobiasdiez commented 1 year ago

@JReinhold I hope its okay to ping you here. Is it currently possible to have stories with a id that conflicts with a standard javascript keyword? For example, is it possible to overwrite the story id with something like this?

export const defaultStory = () => ...
defaultStory.storyName = 'Default'
defaultStory.storyId = 'Default'

I couldn't find anything in this direction, so I went with adding a prefix story to the export. This works, but results in ugly urls like ?path=/story/tests-default-name--storydefault instead of --default at the end.

codecov[bot] commented 1 year ago

Codecov Report

Merging #41 (5fc9b72) into main (e9590e5) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   96.42%   96.47%   +0.05%     
==========================================
  Files           3        3              
  Lines         280      284       +4     
  Branches       45       45              
==========================================
+ Hits          270      274       +4     
  Misses         10       10              
Impacted Files Coverage Δ
src/core/transform.ts 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

JReinhold commented 1 year ago

I don't think there's anything stopping you from using reserved keywords as IDs, I just think that what you're running into here is a limit of the language.

I think this would help if you generated CSF3 stories instead of CSF2.

So

export const defaultStory = () => ...
defaultStory.storyName = 'Default'
defaultStory.storyId = 'Default'

would instead be

export const Default = {
  id: 'default',
  name: 'default',
  render: () => ...
}
tobiasdiez commented 1 year ago

Thanks, migrating to csf3 is a good idea, I wanted to do that anyway.

But I couldn't find anything about the id field in csf3. For example, https://github.com/ComponentDriven/csf/blob/223b0c386f25c708af2159dabf9e75b402723a3e/src/story.ts#L348-L376 lists name but not id. Are you sure that exists?

JReinhold commented 1 year ago

Sorry, I was mixing things up. The ID will be a mix of meta.id and story.name, you should be able to combine them to your liking? It's documented here.

https://storybook.js.org/docs/7.0/vue/configure/sidebar-and-urls#permalink-to-stories

tobiasdiez commented 1 year ago

Thanks for the link, but it's still not clear to me how I would allow users to choose default as the ID.

export const default: Story = {};

clearly doesn't work. Is this just a limitation one has to accept, or is there a nice workaround?

JReinhold commented 1 year ago

The "nice workaround" is to PascalCase all the story exports, so they don't conflict with JS keywords. In general we actually always recommend PascalCase for story exports, probably for stuff like this. The slug will be kebab-cased in the end anyway, so it shouldn't make a difference (I might be missing some cases though)

This works:


const meta = {
  ...
} satisfies Meta<Button>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Default: Story = {
};

export const Class: Story = {
};

export const If: Story = {
};
tobiasdiez commented 1 year ago

Thanks a lot @JReinhold. Using pascalcase worked really well!