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

docs: replaced 'as ComponentMeta' in TypeScript stories #17481

Conversation

natterstefan
Copy link

@natterstefan natterstefan commented Feb 10, 2022

Issue: Unsafe TypeScript examples

What I did

I refactored all TypeScript stories and removed the as ComponentMeta<typeof Example> type assertions. In my opinion, we should use the type as we would other types too (e.g. const foo: Foo = { hello: 'world' }). We should not use as as a way of typing this when it is not the last resort because TypeScript can't infer the type, shouldn't we? 🤔

I am curious to hear what you think. :)

Maybe related PRs and Issues

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Feb 10, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 8c1ad7b. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@natterstefan
Copy link
Author

CircleCi is failing, but I do not think that's related to my change 🤔

image

@ndelangen
Copy link
Member

@natterstefan thank you for the PR.

I agree that this PR improves type safety / strictness which is great.
It does come with a cost of having code in 2 statements vs a single one.

If it wasn't for the above and the fact that this will likely ship very soon in TypeScript:
microsoft/TypeScript#7481

...we'd be merging it. But we don't want to change to something now, and then N months into the future change it again.
This would be annoying to users of storybook, right?
In this instance I'd rather change once, N months from now.

Thank you again for the PR, I can you you're passionate and willing to help. We truly appreciate it!

@ndelangen ndelangen closed this Feb 14, 2022
@natterstefan
Copy link
Author

Hi @ndelangen,

thanks for your feedback and for sharing the link. I wasn't aware of that and makes sense to me. I appreciate the time you invested to share your opinion about this with me.

I will keep an eye on the TypeScript issue and maybe open another PR in the future once they are ready. :)

Until then, thank you for continuing to develop Storybook!

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

Successfully merging this pull request may close these issues.

3 participants