-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 simplified manager.js/preview.js API for addons #17755
Conversation
… separately replacing a single preset file.
…ybookjs/storybook into tech/refactor-simplified-addons-api
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 666f25d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
# Conflicts: # addons/actions/package.json # addons/controls/package.json # addons/interactions/package.json # addons/jest/package.json # addons/toolbars/package.json # examples/web-components-kitchen-sink/yarn.lock # yarn.lock
@ndelangen @shilman this is a much simpler API. Nice work! I have a couple of questions:
import { once } from '@storybook/client-logger';
import './manager';
once.warn(
'register.js is deprecated see https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#deprecated-registerjs'
);
|
@winkerVSbecks thanks for the look!
The warnings are actually implemented per-addon, so this only shows warnings for existing addons. AFAIK this is not a breaking change.
We didn't add it. We modified the existing
Indeed, I'll update the docs as part of this PR. |
const r = name.startsWith('/') ? safeResolve : safeResolveFrom.bind(null, configDir); | ||
const resolved = r(name); | ||
|
||
if (name.match(/\/(manager|register(-panel)?)(\.(js|ts|tsx|jsx))?$/)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we wanna constrain the file extensions? e.g. cjs, mjs
Looks good! But I believe there's a regression with addon-docs. Now every story is loaded in an iframe when opening the docs tab (at least locally in official-storybook, in comparison to the deployed next). Just out of curiosity, this is how a possible "annotations" file would look like when generated with the new format: [
'../../app/react/dist/esm/client/docs/config',
'../../app/react/dist/esm/client/preview/config',
'../../addons/docs/preview.js',
'../../addons/actions/preview.js',
'../../addons/backgrounds/preview.js',
'../../addons/measure/preview.js',
'../../addons/outline/preview.js',
'../../addons/interactions/preview.js',
'../../addons/links/preview.js',
'../../addons/a11y/preview.js',
'preview.js'
] The downside is now that the |
I found out why the regression happened. This is the annotations file before this change:
This is the annotations file after this change:
Notice that before docs was always loading first, so that the framework docs presets could override it correctly! But now the framework presets come first, and get overriden by docs presets |
@yannbf looks like switching the order around does cause issues down the stream :'( |
cf2b7b6
to
8f8f903
Compare
# Conflicts: # MIGRATION.md # addons/actions/package.json # addons/controls/package.json # addons/interactions/package.json # addons/jest/package.json # addons/toolbars/package.json # lib/cli/src/versions.ts # yarn.lock
Going to merge this @ndelangen and we can work on the unrelated CI failures in parallel |
https://www.notion.so/chromatic-ui/Simplified-SB-addons-API-fcabe63977cb47d3959fdbab74b8fc4d
register.js
tomanager.js
preview.js
which is added to the preview annotations by defaultconfig
topreviewAnnotations
for consistency