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

[Tracking]: Remove React-requirement from addons #24490

Closed
9 tasks done
JReinhold opened this issue Oct 17, 2023 · 1 comment
Closed
9 tasks done

[Tracking]: Remove React-requirement from addons #24490

JReinhold opened this issue Oct 17, 2023 · 1 comment

Comments

@JReinhold
Copy link
Contributor

JReinhold commented Oct 17, 2023

This is a tracking issue for removing React as a peer dependency to addons, and maybe more.

Problem

Storybook requires a peer dependency on React and React-DOM, which was added in 7.0 to be compatible with stricter PnP-style package managers like pnpm.

Removing the React as a peer dependency will improve the experience for all non-React users in one shot.

This tracking issue focuses on the addon part while ignoring addon-docs for now, which is also needed to complete this.

Solution

Milestone 1: Investigations

Can we make React a devDependency in `@storybook/manager-api` and friends™️?

We should investigate if we can also make React and the globalized packages devDependencies of our core packages, since they’ll share the same globalization infrastructure as addons.

The biggest caveat here is that we’re essentially making packages “invalid” with this work (this is true for addons as well). The dist output of a package will import React, but React will not be part of the package nor it’s dependents, so it will fail everywhere except for when handled by the manager builder. This is okay, because that’s their intended use anyway.

However we’ll likely run into issues with types. .d.ts files in the dist could reference types from @types/react, but that will not exist if it’s explicitly not bundled in, so the types will be broken. We’ve experienced this problem first hand with an upstream package that didn’t include their transitive types, causing failure.

This is less of a problem for addons, because using their types in anyway is rare, and they will likely not reference React at all.

We should investigate all of this, possible solutions:

  1. Don’t externalize @types/react during the build, causing it to be bundled in.
  2. Run two passes of builds with tsup, one for runtime (ESM, CJS) that externalizes all globalized packages, and one for types (.d.ts) that do not externalize anything.
How will this behave in Yarn PnP, pnpm, and older+newer versions of npm?

We need to investigate how this behaves in the different package managers. Essentially you’ll have packages that in their runtime dist code imports packages that are not part of the dist nor of dependencies. They’ll technically be invalid anywhere outside of the SB Manager.

This is okay as long as they are built with the manager builder before being executed, but are there any situations where a package manager would try to resolve the imports before we start bundling it?

Milestone 2:

We need an escape hatch for the prepublish check though, because there are limited, valid use cases for having React as a peer dependency in an addon - if you’re building an addon that adds components to the preview, like with decorators. These addons should be framework agnostic, but some are React-specific, and they shouldn’t be blocked in a prepublish check.

Documentation

This pattern of not prebundling specific dependencies on purpose is very Storybook-specific and needs to be documented as a callout at https://storybook.js.org/docs/react/addons/writing-addons

It’s important to call out that this change is not strictly necessary or breaking. Any addons that don’t do this migration will still work even with our core changes, they’ll just unnecessarily require users to install React.

@JReinhold
Copy link
Contributor Author

JReinhold commented Oct 30, 2023

Re: How will this behave in Yarn PnP, pnpm, and older+newer versions of npm?

Tested this out in Yarn1, Yarn4, Yarn PnP, npm and pnpm, and none of them complained. I think We're good.

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

No branches or pull requests

1 participant