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

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

Merged
merged 3 commits into from
Apr 15, 2023

Conversation

tobiasdiez
Copy link
Owner

@tobiasdiez tobiasdiez commented Mar 19, 2023

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
Copy link
Owner Author

@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
Copy link

codecov bot commented Mar 19, 2023

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%> (ø)

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

@JReinhold
Copy link

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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

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
Copy link

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 tobiasdiez mentioned this pull request Apr 6, 2023
@tobiasdiez
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected token error
2 participants