storybookjs / eslint-plugin-storybook

🎗Official ESLint plugin for Storybook
MIT License
245 stars 52 forks source link

`prefer-pascal-case` and `story-exports` throw error if function's `name` property is used in `includeStories`/`excludeStories` arrays. #65

Closed ignas-sedunovas closed 2 years ago

ignas-sedunovas commented 2 years ago

Describe the bug prefer-pascal-case and story-exports throw error if function's name property is used in includeStories/excludeStories arrays.

To Reproduce Exclude or include a story using the name property of it. Example:

export default {
  title: "MyComponent",
  component: MyComponent,
  excludeStories: [MyComponent.name]
}

function MyComponent() {}

Produces an error such as:

Error: Unexpected descriptor element: MemberExpression
Occurred while linting C:\fake-path\MyComponent.stories.tsx:16
    at C:\fake-path\node_modules\eslint-plugin-storybook\dist\utils\index.js:39:27
    at Array.map (<anonymous>)
    at getDescriptor (C:\fake-path\node_modules\eslint-plugin-storybook\dist\utils\index.js:37:44)
    at ExportDefaultDeclaration (C:\fake-path\node_modules\eslint-plugin-storybook\dist\rules\prefer-pascal-case.js:104:67)
    at C:\fake-path\node_modules\eslint\lib\linter\safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (C:\fake-path\node_modules\eslint\lib\linter\safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (C:\fake-path\node_modules\eslint\lib\linter\node-event-generator.js:293:26)
    at NodeEventGenerator.applySelectors (C:\fake-path\node_modules\eslint\lib\linter\node-event-generator.js:322:22)
    at NodeEventGenerator.enterNode (C:\fake-path\node_modules\eslint\lib\linter\node-event-generator.js:336:14)
Process finished with exit code -1

Expected behavior ESLint does not crash

Additional context It seems that the problem is at https://github.com/storybookjs/eslint-plugin-storybook/blob/main/lib/utils/index.ts#L48, because it doesn't expect such usage as in my example. AST of my example produces a PropertyAccessExpression. It's valid string, but only at runtime.

I'm using such approach in order to automatically rename the story also in includeStories/excludeStories arrays when refactoring.

Would you consider expanding the linter rules to allow such approach?

shilman commented 2 years ago

Thanks @ignas-sedunovas! Here's my quick $.02:

The linter shouldn't crash, so we should add that test case and make sure it's handled gracefully

Doing any kind of dynamic analysis is complicated, so I think we should just ignore include/exclude stories that are not literals. If the user wants to disable the rule for a particular export they can do that on the line above the export, or they can just disable the rule altogether.

@yannbf WDYT?

yannbf commented 2 years ago

Thanks @ignas-sedunovas! Here's my quick $.02:

The linter shouldn't crash, so we should add that test case and make sure it's handled gracefully

Doing any kind of dynamic analysis is complicated, so I think we should just ignore include/exclude stories that are not literals. If the user wants to disable the rule for a particular export they can do that on the line above the export, or they can just disable the rule altogether.

@yannbf WDYT?

I agree. I will handle the case so the plugin doesn't crash. Thank you so much for opening this issue @ignas-sedunovas!

yannbf commented 2 years ago

Hey @ignas-sedunovas this should now be fixed in v0.5.5.

Thanks for using this library! <3