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

Addons, core: Make react and Storybook packages devDependencies where possible #24676

Merged
merged 28 commits into from
Nov 13, 2023

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Nov 2, 2023

Works on #24490

TODO

  • Document new optimizeDeps preset property
  • Reword "no React" error
  • Test that an addon author can import from manager-api and still have correct types, even though @storybook/types and @types/react are not part of the package.

What I did

This PR is a retry of #23486 with many non-essential things ignored.

The main purpose of this PR is to minimize the amount of peer dependencies in addons and a few of the core packages, most notably removing react as a peer dependency.

The way the manager and the preview globalizes a fixed set of dependencies means that these packages don't strictly need the dependencies, as they will be referenced from the global window instead, which our builders ensures. Removing them from peer dependencies mean that users don't need to specify them either.

Having them as devDependencies will however make our (tsup) bundler default to bundle them in, which we don't want. Therefore this PR also introduces a new bundler configuration, addon-bundle that makes sure that the globalized packages are externalized (thus the imports are kept intact), so they can be replaced with the global reference when built.
A handful of packages like manager-api are not using addon-bundle but still need these packages externalized, so they've been added to their bundler configuration manually to ensure that happens.

addon-essentials and addon-docs are largely left out of this PR, as tackling them in terms of react requires more work.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

It's important to test this in the different package manager configuration, npm, yarn, yarn pnp and pnpm and with both builders.

For each configuration and builder:

  1. Create a new non-React sandbox outside of the monorepo (npx sb@next sandbox)
  2. Replace all versions with this PR's canary version.

Testing addon-essentials with docs

  1. Run storybook and build-storybook and see that it works

Testing addon-docs directly

  1. Replace addon-essentials with addon-docs both in main.ts and package.json
  2. Run storybook and build-storybook and see that it works

Testing addon-essentials with docs disabled

  1. Remove the mdx glob from stories in main.ts
  2. Replace addon-docs with addon-essentails both in main.ts and package.json again
  3. Remove react and react-dom from package.json. You will see an unmet peer deps from addon-essentials, that's okay.
  4. disable docs in main.ts with { name: '@storybook/addon-essential', options: { docs: false } }
  5. Run storybook and build-storybook and see that it works, now without React

Testing without addon-essentials and addon-docs

  1. Remove addon-docs with addon-essentails both from main.ts and package.json again
  2. Run storybook and build-storybook and see that it works

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-24676-sha-214a6f84. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-24676-sha-214a6f84
Triggered by @JReinhold
Repository storybookjs/storybook
Branch experiment-addon-peer-dep
Commit 214a6f84
Datetime Wed Nov 8 11:20:01 UTC 2023 (1699442401)
Workflow run 6797494862

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24676

@JReinhold
Copy link
Contributor Author

@shilman could I get you to run benchmarks for this branch/version? It's largely complete now.

We expect unchanged performance and build size, but we expect a noticeable reduction in install size.

Comment on lines +35 to +36
"./manager": "./dist/manager.js",
"./register": "./dist/manager.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

manager and preview entries don't need types nor cjs outputs as they are always loaded in the manager or preview in the browser, never directly imported by the user.

Comment on lines +74 to +79
"exportEntries": [
"./src/index.ts"
],
"platform": "browser"
"managerEntries": [
"./src/manager.tsx"
]
Copy link
Contributor Author

@JReinhold JReinhold Nov 2, 2023

Choose a reason for hiding this comment

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

these new fields are used by addon-bundle.ts to determine which entry points needs which packages externalized.
exportEntries are assumed to be imported by users, so they won't have any externalization and they'll keep their d.ts files

Comment on lines 197 to 203
const optimizeDeps = [
'@mdx-js/react',
'@storybook/addon-docs > acorn-jsx',
'@storybook/addon-docs',
'@storybook/addon-essentials/docs/mdx-react-shim',
'markdown-to-jsx',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from the Vite builder's default optimization, as that would break if addon-essentials was installed without react, which is a valid scenario now.

@JReinhold
Copy link
Contributor Author

@shilman benchmark comparisons between next as a baseline (top) and this branch (bottom):

I don't know how much of this is noise and how much is actual change.

Vite

headers
next
this branch

Webpack

headers
next
this branch

"@testing-library/react": "^11.2.2",
"lodash": "^4.17.21",
"react": "^16.8.0",
"react-dom": "^16.8.0",
Copy link
Member

Choose a reason for hiding this comment

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

Did you check if the react-dom dep is needed truly in these addons?

It's not a big deal at all having them as devDependencies of course, no harm to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I added it as a todo to my cooldown period.

  • Remove react-dom from package.jsons when not needed
  • Audit dependencies of frameworks+renderers, they might have a lot of unused @storybook dependencies we can remove

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.

I only reviewed for perf. I found that the sizes were slightly worse across the board, but not by an amount large enough to block the work. It would be useful to explain why the sizes all increased by a small amount, though, since it's not intuitive to me based on the change.

@JReinhold JReinhold merged commit 657f389 into next Nov 13, 2023
60 checks passed
@JReinhold JReinhold deleted the experiment-addon-peer-dep branch November 13, 2023 10:59
@github-actions github-actions bot mentioned this pull request Nov 13, 2023
30 tasks
@kasperpeulen kasperpeulen restored the experiment-addon-peer-dep branch November 13, 2023 19:59
@kasperpeulen kasperpeulen added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Nov 13, 2023
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.

4 participants