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

Add meta-satisfies-type rule #124

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NotWoods
Copy link

@NotWoods NotWoods commented Apr 17, 2023

Issue: #123

What Changed

Added a new rule to check that meta is always followed by satisfies Meta.

This probably needs a new configuration (ie csf-typescript) for TypeScript-only rules. I'm not sure if ESLint has a way to check the file extension or not.

Checklist

Check the ones applicable to your change:

  • Ran yarn update-all
  • Tests are updated
  • Documentation is updated

Change Type

Indicate the type of change your pull request is:

  • maintenance
  • documentation
  • patch
  • minor
  • major

@yannbf
Copy link
Member

yannbf commented Apr 18, 2023

Hey @NotWoods thanks a lot for your contribution! Looks pretty cool. I think it needs more thought indeed, regarding a new category or something, as we don't want this to be enforced by default. We could potentially introduce the rule disabled by default, so users can enable when they want.

@kasperpeulen I believe you might be interested in this!

@NotWoods
Copy link
Author

I removed the current requirement that a rule has at least one category. That should let this rule be included without getting enabled by default :)

@kasperpeulen
Copy link
Contributor

Looks good!

@NotWoods
Copy link
Author

Hello! Just checking in, is there any changes I need to make to this rule?

@kasperpeulen
Copy link
Contributor

cc @yannbf

@hjoelh
Copy link
Contributor

hjoelh commented Aug 8, 2023

Looks great @NotWoods 🚀

Do you think we'd need a separate rule for enforcing StoryObj<typeof meta> instead of StoryObj<typeof YourComponent> to get full safety? (thats the only way I can get TS to complain about required props/args)

Btw, it looks like the pr contains a duplicate test file?
tests/lib/rules/meta-safisfies-type.test.ts
tests/lib/rules/meta-satisfies-type.test.ts

@NotWoods
Copy link
Author

NotWoods commented Aug 8, 2023

Yeah, I think that should be added as a separate rule. I'll look into fixing the duplicate file.

@yannbf
Copy link
Member

yannbf commented Aug 15, 2023

Hey there, I'll be checking this with @kasperpeulen this week, thanks for your patience 🙏

@NotWoods NotWoods force-pushed the meta-satisfies-type branch from 74f90e8 to 3701640 Compare August 16, 2023 04:05
@NotWoods
Copy link
Author

NotWoods commented Feb 2, 2024

Hi folks, are there any changes I need to make here?

@opii972
Copy link

opii972 commented Dec 2, 2024

Definitely a game changer with TypeScript because a story that implements a component with empty props allows wrong args.

Example:

For this component:

const Comp: React.FC<Record<string, never>> = () => <>Lorem ipsum</>

export default Comp

Note: React.FC<Record<string, never>> instead React.FC<{}> (or simply React.FC) because {} in Typescript actually means "any non-nullish value"

Without this rule (valid code):

export default {
  title: 'Comp',
  args: { primary: true },
  component: Comp,
}

Or

export default {
  title: 'Comp',
  args: { primary: true },
  component: Comp,
} satisfies Meta

With this rule (invalid code):

export default {
  title: 'Comp',
  args: { primary: true }, // <-- throws an error
  component: Comp,
} satisfies Meta<typeof Comp>

@Sidnioulz Sidnioulz self-assigned this Feb 11, 2025
@Sidnioulz Sidnioulz self-requested a review February 11, 2025 11:41
Copy link
Member

@Sidnioulz Sidnioulz left a comment

Choose a reason for hiding this comment

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

Great work @NotWoods! 🤩 Looks complete to me.

I've created a rebased branch on the storybook repo which removes category-related changes that were no longer necessary, adjusts your code to account for other changes, and makes opinionated minor meta/docs edits.

Feel free to use what's on it to update your code, or to merge it into your branch. Do let us know too if you're ok with closing this PR and merging the other branch instead (I've preserved the author information of course).

@@ -114,6 +114,7 @@ This plugin does not support MDX files.
| [`storybook/csf-component`](./docs/rules/csf-component.md) | The component property should be set | | <ul><li>csf</li></ul> |
| [`storybook/default-exports`](./docs/rules/default-exports.md) | Story files should have a default export | 🔧 | <ul><li>csf</li><li>recommended</li></ul> |
| [`storybook/hierarchy-separator`](./docs/rules/hierarchy-separator.md) | Deprecated hierarchy separator in title property | 🔧 | <ul><li>csf</li><li>recommended</li></ul> |
| [`storybook/meta-satisfies-type`](./docs/rules/meta-satisfies-type.md) | Meta should use `satisfies Meta` | 🔧 | (none) |
Copy link
Member

Choose a reason for hiding this comment

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

Replace with N/A to align with the new meta-inline-properties rule's documentation.

Suggested change
| [`storybook/meta-satisfies-type`](./docs/rules/meta-satisfies-type.md) | Meta should use `satisfies Meta` | 🔧 | (none) |
| [`storybook/meta-satisfies-type`](./docs/rules/meta-satisfies-type.md) | Meta should use `satisfies Meta` | 🔧 | N/A |


<!-- RULE-CATEGORIES:START -->

**Included in these configurations**: (none)
Copy link
Member

Choose a reason for hiding this comment

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

Align with meta-inline-properties

Suggested change
**Included in these configurations**: (none)
**Included in these configurations**: N/A


Examples of **incorrect** code for this rule:

```js
Copy link
Member

Choose a reason for hiding this comment

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

Rules should probably be separated for legibility, and use ts rather than js highlighting.

Comment on lines +18 to +30
meta: {
type: 'suggestion',
docs: {
description: 'Meta should use `satisfies Meta`',
categories: [],
recommended: 'error',
},
messages: {
metaShouldSatisfyType: 'Meta should be followed by `satisfies Meta`',
},
fixable: 'code',
schema: [],
},
Copy link
Member

@Sidnioulz Sidnioulz Feb 11, 2025

Choose a reason for hiding this comment

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

I've had to make adjustments while rebasing:

  • there's now a meta.docs.excludeFromConfig key for uncategorised
  • severity has been added to report a rule's severity

I've also made opinionated edits on my own branch while rebasing:

  • Switched type from suggestion to problem as another user pointed out a case where using the code pattern promoted by this rule helped TS report invalid args content
  • Switched message to 'CSF Meta should use satisfies for type safety' so it explains that satisfies improves type safety for users not familiar with it

Comment on lines +33 to +35
rule.meta.docs.categories && rule.meta.docs.categories.length > 0
? `<ul>${rule.meta.docs.categories.map((c) => `<li>${c}</li>`).join('')}</ul>`
: '',
: '(none)',
Copy link
Member

Choose a reason for hiding this comment

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

This part is no longer needed now as there is a mechanism to ignore rules without categories, and I've been able to remove it entirely on my rebased branch.

Comment on lines 25 to 32

for (const rule of rules) {
const ruleCategories = rule.meta.docs.categories
// Throw if rule does not have a category
if (!ruleCategories?.length) {
throw new Error(`Rule "${rule.ruleId}" does not have any category.`)
}

if (ruleCategories.includes(categoryId)) {
if (ruleCategories?.includes(categoryId)) {
categoriesConfig[categoryId].rules?.push(rule)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This part is no longer needed now as there is a mechanism to ignore rules without categories, and I've been able to remove it entirely on my rebased branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Empathy Backlog
Development

Successfully merging this pull request may close these issues.

6 participants