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

Essentials: Prebundle addons and minimize need for react/react-dom as peerDependency #23486

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jul 17, 2023

What I did

  • I introduce a new bundle script specific for addons.
    • their preview.ts and manager.ts entrypoints never need to have types generated for them, not generating them saves install-size and compile/publish-time
    • their manager.ts file should be prebundled. References to manager-related storybook packages and react will be globalized/externalized by the builder-manager.. So why not do this before publishing.
    • their preview.ts file should be prebundled. References to preview-related storybook packages will be globalized by the builder.. So why not do this before publishing?
    • their preset.ts should be compiled for node16 only, and does not need types generated.
    • their other entrypoints should be compiled for both the browser and node16, and should have types generated, because these are often for public use, such as importing decorators.
  • I migrate all core addons to use this new pattern
  • I removed react & react-dom peerDependencies as much as possible.
    • addons that depend on react because of manager.ts injecting custom Panels and such do not need the dependency anymore, this is solved via the new bundle script for addons!
    • addons that depend on react for decorators will need a peerDependency on react. This makes the addon react specific, though a notable exception here is addon-links. It has a react dep for the LinkTo component, that is it's own export... I've made this have an optional peerDependency on react for now. Such addons ARE NOT ALLOWED IN ESSENTIALS!
    • builder-webpack5 needs react because it added aliases for some manager-packages. I removed this here: WebpackBuilder: Remove need for react as peerDependency #23496

To be determined

  • react in the preview

    • TBD: addon's preview.ts file might have a dependency to react, I could possibly prebundle this as well, and maybe globalized/externalized react for the preview?
    • TBD: should I try to externalize/globalize react, and possibly be able to prebundle most if not all of addon-docs? I could make a prebundled version of react and inject it into the preview, so the global variable is available. If the user isn't using react themselves, I could use the version storybook shipped with, if the user does use react themselves, I'd quickly prebundle their version of react, and inject that...
  • react in manager deps

    • manager-api needs react because it exposes components and hooks. This package ends up being embedded into managerpackage, in which react is prebundled. I've spiked moving the code into manager: Refactor: Move code of lib/manager-api into ui/manager #23389. That PR doesn't actually change the fact the package is depending on react. Users of this package (addon-authors) will want to get proper types, which include react-types! Embedding those in might actually be the correct move, because that would ensure users will be told if they tried to do things our version of react embedded into the manager can't do...
    • router needs react because it's embedding react-router. This package is only used in the manager, so whatever I end up doing to manager-api I'll likely do here as well. I'm not sure this packages has any real-world use anymore TBH, could be merged into manager-api?
    • theming needs react because it's embedding emotion-react. This package is used in the manager, but has seen other uses as well. globalizing react in it's dist seems not possible to me because of it.
  • TBD: How to cope with manager-api, router and theming packages depending on react.

    • I am considering making these dependencies normal dependencies instead of peerDependencies.
    • I am considering making them optional peerDependencies via peerDependenciesMeta
    • I am considering (possibly in addition to one of the above 2) to create another bundle script for these packages specifically to create a browser extra export. I'm not sure what the point of that would truly be.
    • Consider moving manager-api's and or router's implementations into the manager package.
  • TBD: How to cope with blocks and components packages depending on react.

    • I'm considering making them have optional peerDependencies on react
    • components also has a dependency on react-dom.
  • TBD: Do we want to remove addon-docs from sb init?

  • TBD: Do I remove the addon-links LinkTo component? ? move it somewhere else to make addon-links not depend on react anymore? Do I externalize react in the preview?

  • TBD: the package types needs react, mostly for manager-api stuff. It's externalized it seems right now, without any dependency defined.. this is possibly problematic, but considering it's just types, and the recommendation is skipLibCheck this might be ok??

    • Perhaps these types actually belong in manager-api?

How to test

TBD

@ndelangen ndelangen changed the title Essentials Essentials: Prebundle addons and remove need for react as peerDependency Jul 17, 2023
@socket-security
Copy link

socket-security bot commented Jul 18, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@socket-security
Copy link

socket-security bot commented Jul 18, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

…into norbert/no-docs-no-react-peers

# Conflicts:
#	code/addons/a11y/package.json
#	code/addons/actions/package.json
#	code/addons/backgrounds/package.json
#	code/addons/controls/package.json
#	code/addons/essentials/package.json
#	code/addons/gfm/package.json
#	code/addons/highlight/package.json
#	code/addons/interactions/package.json
#	code/addons/jest/package.json
#	code/addons/links/package.json
#	code/addons/measure/package.json
#	code/addons/outline/package.json
#	code/addons/storysource/package.json
#	code/addons/toolbars/package.json
#	code/addons/viewport/package.json
@ndelangen ndelangen requested a review from shilman July 19, 2023 09:47
@ndelangen
Copy link
Member Author

Notes on how to globalize react & react-dom inside of the preview.

  • I can create a prebundled version of both packages
    • Where does this code go? The builder should be able to access this?
    • Preferably we do not create any new packages.
  • How is this code injected into the builder?
    • Preferably this is builder-agnostic, I could use previewHead, but the vite builder has a nasty habit of using the HTML as it's entrypoint, thus it would possibly embed/bundle it, or maybe this is not problematic?
    • How to ensure this code is fully run before any other preview runtime code?
    • How to ensure the builders will externalize react and react-dom
  • Is the user using react?
    • Is this dependent on the dependency being installed?
      How to properly detect this, consider pnp and pnpm!
    • Maybe it's based on the framework/renderer being used?
      This should be relatively easy to reliably detect with preset.apply('framework')?
  • When the user is NOT using react, we should inject our prebundled blob
  • When the user IS using reactwe'll need to quickly prebundle their version.
    • Can this be done in a pnp or pnpm environment?
    • Where do we write the script to do this? core-common?

If we solve the above, I can make a version of addon-docs, that has everything prebundled in, and still uses the globalized version of the user's react and react-dom.

In addition I could adjust the addon-bundler to also globalize react and react-dom for preview entries.

@ndelangen ndelangen changed the title Essentials: Prebundle addons and remove need for react as peerDependency Essentials: Prebundle addons and minimize need for react/react-dom as peerDependency Jul 19, 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.

2 participants