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: Remove unnecessary peer deps #20231

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

ndelangen
Copy link
Member

Issue: #20179

What I did

Be confused, as to why we're getting the errors in the issue at all.
But I was able to clean up the peerDeps a bit.

@ndelangen ndelangen changed the title cleanup up some peerDeps Fix: cleanup up some peerDeps Dec 13, 2022
@ndelangen ndelangen self-assigned this Dec 13, 2022
@ndelangen ndelangen added the bug label Dec 13, 2022
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

I don't think this completely addresses the linked issue, and I have a question, but it looks like good changes to me.

@@ -77,11 +77,6 @@
"type-fest": "^2.19.0",
"typescript": "~4.9.3"
},
"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.

Why is typescript in devDependencies and not dependencies? Does that mean it's being bundled into @storybook/core-common?

Copy link
Member Author

@ndelangen ndelangen Dec 13, 2022

Choose a reason for hiding this comment

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

Does that mean it's being bundled into @storybook/core-common?

No, not necessarily, only if the bundler (tsup) would be able to find a dependency chain from any entrypoint to the typescript module. Which if that happened I would find very strange because why would core-common import typescript as a runtime dependency anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Right, okay, I keep forgetting that part of things, thanks for being patient with me.

Copy link
Member Author

@ndelangen ndelangen Dec 13, 2022

Choose a reason for hiding this comment

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

Why is typescript in devDependencies and not dependencies?

Because if it defined it as a regular dependency, it would also add that module as a dependency for users to install, which I don not think is something we want, right?

Another question would be: "Why is it listed as a devDependency?" to which the answer is:
It's likely isn't, but we do have TS code in the source, so it also makes sense to have it?

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.

Good to get rid of builder-webpack5.

I'm confused about the typescript changes though. Why does making it a required peer dependency help anything? I would think it would have the opposite effect.

@IanVS
Copy link
Member

IanVS commented Dec 14, 2022

Why does making it a required peer dependency help anything? I would think it would have the opposite effect.

TypeScript isn't in the peerDependencies of these packages.

@ndelangen
Copy link
Member Author

@shilman It's an educated guess of mine but:

If this package has a peerDependency on typescript and declares it's an optional... than perhaps that will satisfy the package manager.

It's just a theory, it could be the packageManager will still traverse up, and find the peerDep is unsatisfied..

I'm guessing the only solution then would be to go into the "offending" package: @joshwooding/vite-plugin-react-docgen-TypeScript and make it the peerDependency optional there. cc. @joshwooding

@shilman shilman changed the title Fix: cleanup up some peerDeps Core: Remove unnecessary peer deps Dec 14, 2022
@shilman shilman merged commit afcd3b4 into next Dec 14, 2022
@shilman shilman deleted the norbert/sb-1087-sb20179-pnpm-missing-typescript-peer branch December 14, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants