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

Core: Add typescript as optional peer dependency #13330

Merged
merged 3 commits into from Dec 1, 2020

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Nov 29, 2020

Issue: TypeScript is a peer dependency for fork-ts-checker-webpack-plugin and react-docgen-typescript-plugin, but isn't provided by neither @storybook/core nor @storybook/react, and thus generates warnings:

➤ YN0002: │ @storybook/core@npm:6.1.0-beta.7 [7b568] doesn't provide typescript (pa4c6c), requested by fork-ts-checker-webpack-plugin
➤ YN0002: │ @storybook/react@npm:6.1.0-beta.7 [61f66] doesn't provide typescript (p56aea), requested by react-docgen-typescript-plugin

What I did

Since Yarn requires peer dependencies to be listed at every level of dependency chains, I've added TypeScript as an optional peer dependency of both @storybook/core and @storybook/react, so that they can forward the package to their own dependencies.

How to test

n/a

@@ -67,6 +67,11 @@
"react": "^16.8.0 || ^17.0.0",
"react-dom": "^16.8.0 || ^17.0.0"
},
"peerDependenciesMeta": {
"typescript": {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't typescript be listed as peerDependencies too? Or having this peerDependenciesMeta info is enough and Yarn will "do it" for us under the hood?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it would be more correct to list it, yes. However when we introduced the feature some maintainers were concerned that it would print more warnings on older package managers (for example for their users running npm 5), so we improved it by making peerDependenciesMeta imply a corresponding peer dependency on * (this way older package managers just ignore the peer dependency). So in short, it's as you prefer, we support both 🙂

@gaetanmaisse gaetanmaisse added the maintenance User-facing maintenance tasks label Nov 29, 2020
@shilman shilman changed the title Adds typescript as optional peer dependency Core: Add typescript as optional peer dependency Dec 1, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @arcanis 🙏

@shilman shilman merged commit bbbb4c8 into storybookjs:next Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants