-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Core: Add experimental_devServer preset
#32862
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
Conversation
📝 WalkthroughWalkthroughThe changes enable an experimental dev server preset hook that allows addons and frameworks to extend the dev server with custom middlewares. The ServerApp interface is exported to support a new optional experimental_devServer configuration option in StorybookConfigRaw. Changes
Sequence DiagramsequenceDiagram
participant DevServer as Dev Server Init
participant Middleware as Middleware Setup
participant Preset as Presets Manager
participant Addons as Addons/Frameworks
participant App as Polka App
DevServer->>Middleware: Wire middleware
Middleware-->>DevServer: Complete
Note over DevServer: NEW: Apply experimental preset
DevServer->>Preset: apply('experimental_devServer', app)
Preset->>Addons: Check for registered preset
Addons-->>Preset: Return extension handler
Preset->>App: Execute extension (add custom middleware)
App-->>Preset: Done
Preset-->>DevServer: Preset applied
DevServer->>DevServer: Destructure port/host/initialPath
DevServer->>App: Start listening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/types/modules/core-common.ts (1)
502-601: Missing experimental_devServer from public StorybookConfig interface.The
experimental_devServerproperty is added toStorybookConfigRawbut not to the publicStorybookConfiginterface. This means users cannot use it in theirmain.tsconfiguration files with proper TypeScript support. Looking at the pattern of other experimental properties likeexperimental_indexers(line 481 in Raw, line 568 in Config), this should be included in both interfaces.Add the following property to the
StorybookConfiginterface after line 568:/** Extend the dev server with custom middlewares and handlers (experimental) */ experimental_devServer?: PresetValue<StorybookConfigRaw['experimental_devServer']>;Note: This assumes the type fix for
experimental_devServerinStorybookConfigRawis applied first.
🧹 Nitpick comments (1)
code/core/src/types/modules/core-common.ts (1)
64-109: Consider adding a Presets interface overload for experimental_devServer.While the generic
apply<T>overload will handle this at runtime, adding a specific overload would provide better type safety and IDE autocomplete support, consistent with other preset extensions.After line 108, add:
apply( extension: 'experimental_devServer', config?: (app: ServerApp) => ServerApp | Promise<ServerApp>, args?: any ): Promise<ServerApp>;Note: This assumes the type fix in line 483 is applied first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/dev-server.ts(1 hunks)code/core/src/types/modules/core-common.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/types/modules/core-common.tscode/core/src/core-server/dev-server.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/types/modules/core-common.tscode/core/src/core-server/dev-server.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/types/modules/core-common.tscode/core/src/core-server/dev-server.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/types/modules/core-common.tscode/core/src/core-server/dev-server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: get-branch
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/types/modules/core-common.ts (1)
228-243: LGTM! ServerApp export is appropriate.Exporting the
ServerAppinterface is necessary for the newexperimental_devServerconfiguration option. The interface properly defines the Polka-like API surface.
| // Apply experimental_devServer preset to allow addons/frameworks to extend the dev server with middlewares, etc. | ||
| await options.presets.apply('experimental_devServer', app); |
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.
Type mismatch and missing return value capture.
Based on the PR objectives, experimental_devServer should be an async function that receives the app and returns it (e.g., async (app) => { app.use(...); return app; }). However, the type definition in core-common.ts line 483 defines it as experimental_devServer?: ServerApp, not as a function type. Additionally, the return value from the preset application is not captured, which means if a preset returns a modified app instance, it will be lost.
Apply this diff to capture the return value:
// Apply experimental_devServer preset to allow addons/frameworks to extend the dev server with middlewares, etc.
- await options.presets.apply('experimental_devServer', app);
+ const modifiedApp = await options.presets.apply('experimental_devServer', app);
+ // Note: Currently presets are expected to mutate the app in place, but capturing the return value
+ // allows for future flexibility if presets need to return a new instanceAdditionally, update the type definition in core-common.ts to reflect that this should be a function:
experimental_devServer?: (app: ServerApp) => ServerApp | Promise<ServerApp>;🤖 Prompt for AI Agents
In code/core/src/core-server/dev-server.ts around lines 53-54, the preset
application call awaits options.presets.apply('experimental_devServer', app) but
does not capture a returned (possibly modified) ServerApp; change the call to
capture and reassign the result (e.g., app = await options.presets.apply(...))
so any modified app is preserved. Also update the type in core-common.ts at line
~483 to declare experimental_devServer as a function type:
experimental_devServer?: (app: ServerApp) => ServerApp | Promise<ServerApp>, so
the preset is correctly typed as an async function that receives and returns the
app.
|
|
||
| experimental_indexers?: Indexer[]; | ||
|
|
||
| experimental_devServer?: ServerApp; |
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.
Incorrect type definition for experimental_devServer.
The type is defined as ServerApp directly, but based on the PR objectives, this should be a function that receives the ServerApp and returns it (potentially modified). The example usage shows:
experimental_devServer: async (app) => {
app.use('/my-custom-route', ...);
return app;
}Apply this diff to fix the type:
- experimental_devServer?: ServerApp;
+ experimental_devServer?: (app: ServerApp) => ServerApp | Promise<ServerApp>;🤖 Prompt for AI Agents
In code/core/src/types/modules/core-common.ts around line 483, the
experimental_devServer field is incorrectly typed as ServerApp; change its type
to a function signature that accepts a ServerApp and returns either a ServerApp
or a Promise<ServerApp> (e.g. (app: ServerApp) => ServerApp |
Promise<ServerApp>), and update any related imports/types if necessary so
callers like async (app) => { app.use(...); return app; } type-check correctly.
|
View your CI Pipeline Execution ↗ for commit 7a5bb32
☁️ Nx Cloud last updated this comment at |
Core: Add `experimental_devServer` preset (cherry picked from commit 6bb3f61)
Closes #
What I did
Bringing the work from #32816 into 10.0.
Here's the OG description:
Added a new preset property,
experimental_devServer, that is very similar in nature toexperimental_serverChannel, in that it allows any addon/etc. to get access to the dev server instance. This is useful for attaching middlewares and other stuff to the dev server.We want this so that
@storybook/addon-mcpcan attach a handler on the/mcproute without using the current Vite-based workaround.With this, addons should be able to do:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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.Summary by CodeRabbit