feat:unified withStorybook wrapper for entrypoint-swapping#871
feat:unified withStorybook wrapper for entrypoint-swapping#871
withStorybook wrapper for entrypoint-swapping#871Conversation
🦋 Changeset detectedLatest commit: 706b49a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@copilot, can you try again? It looks like you had some internal error. |
…overrides to withStorybook M4 implementation: - Add resolveEntryPoint() to detect app entry (Expo, Expo Router, RN CLI) - Add entry-point swapping in resolver when STORYBOOK_ENABLED=true - Add websocket env variable overrides (STORYBOOK_WS_HOST/PORT/SECURED) - Add comprehensive tests for all new functionality Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/58a5fb17-1ee3-4e0b-ae60-aee474de2d32 Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/58a5fb17-1ee3-4e0b-ae60-aee474de2d32 Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
Implemented M4 in commits a889fb4 and 537de98. The changes add:
|
|
@copilot Did you catch that we'd need backwards support? I think it'd be best if we created 2 separate |
- Restore withStorybook to original behavior (no entry-point swapping, no ws env overrides) - Create withStorybookSwap with entry-point swapping + ws env overrides - Export shared utilities (resolveEntryPoint, resolveFileWithExtensions, resolveStorybookEntry, applyWebsocketEnvOverrides) from withStorybook.ts - Add metro/withStorybookSwap.js re-export and package.json export path - Add withStorybookSwap to tsup build entries - Tests: 33 passing across 5 suites Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/a3213ee7-9a44-4b9a-a4b2-f434af51c3ce Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/a3213ee7-9a44-4b9a-a4b2-f434af51c3ce Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
|
@copilot I'm aiming for the new config wrapper to be a single agnostic one, exposed as This exports a single named export ( The types of this function should be like so: Though internally we should try to be somewhat type-safe, but externally it's not exposed. |
…/withStorybook Creates a bundler-agnostic withStorybook<T>(config: T, options?): T that auto-detects Metro vs Repack and delegates accordingly: - Metro: entry-point swapping + ws env overrides (via withStorybookSwap) - Repack: adds StorybookPlugin with ws env overrides to plugins array This allows sb init to always generate the same config regardless of bundler. Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/e08bd14d-760c-471e-938c-e26d89e9ae9b Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/e08bd14d-760c-471e-938c-e26d89e9ae9b Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
Added a unified
Usage is identical regardless of bundler: const { withStorybook } = require('@storybook/react-native/withStorybook');
module.exports = withStorybook(config);The existing |
|
@copilot the previously existing ./metro/withStorybook and ./repack/withStorybook ARE the backwards support, and should remain completely untouched. The |
Agent-Logs-Url: https://github.com/storybookjs/react-native/sessions/d02c8d49-63e9-4f39-8100-da83eff9ff85 Co-authored-by: ndelangen <3070389+ndelangen@users.noreply.github.com>
- Updated the enhanceMetroConfig function to return an empty type for 'tty' and 'os' module requests, preventing potential resolution issues. - This change improves compatibility and stability in module resolution without altering existing functionality. All tests pass successfully.
…om:storybookjs/react-native into copilot/add-entry-point-swapping-storybook
…-swapping-storybook
- Modified the warning message to clarify that entries in the `addons` field should be moved to `deviceAddons`, emphasizing that they will not be evaluated as Storybook Core presets. - Removed redundant text for improved readability. This change aims to enhance user understanding of the deprecation notice.
- Ensured that both `package.json` files in the root and `react-native` package have a newline at the end of the file for better compatibility with various tools and standards.
|
|
||
| const mainPathTs = path.resolve(cwd, configPath, `main.ts`); | ||
| const mainPathJs = path.resolve(cwd, configPath, `main.js`); | ||
| const mainPathCjs = path.resolve(cwd, configPath, `main.cjs`); |
There was a problem hiding this comment.
nitpick: starting to get the feeling this could be improved with getFilePathWithExtension from common.js
| // Expo Router detection: if main points to expo-router/entry, resolve from node_modules | ||
| if (mainField === 'expo-router/entry') { | ||
| const expoRouterEntry = resolveFileWithExtensions( | ||
| path.resolve(projectRoot, 'node_modules', 'expo-router', 'entry'), |
There was a problem hiding this comment.
I don't think we can assume the path will be projectRoot/node_modules we're not taking into account monorepo setups where it could be projectRoot/../../node_modules or many other different locations.
There was a problem hiding this comment.
@ndelangen this seems like an important issue to resolve
There was a problem hiding this comment.
require.resolve might be better?
|
another thing is that the example app in this repo doesn't have an example of using this setup, it would be nice to have a way to test it here enabled=false still shows me storybook in this repo, probably because in this case we don't have a 'fallback' app 
this is when I run with
i.e no env vars set could you add some kind of simple way to test it on the example app? |
- Updated the withStorybook function to allow the enabled state to be determined by options.enabled, providing greater flexibility in configuration. - This change ensures that the function behaves correctly based on the provided options, improving usability for developers.
|
@dannyhw I cannot reproduce the problem you described with the Maybe I fixed it with my last commit. |
|
@dannyhw this #871 (comment) should be fixed now too. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/react-native/src/View.tsx:24
react-native-safe-area-contextis declared as an optional peer dependency in this package, but it’s now imported unconditionally. Projects that don’t install it will fail to bundle/run with a module-not-found error. Either make it a required dependency/peer (remove it frompeerDependenciesMeta.optional) or make the import optional with a runtime fallback (e.g., useSafeAreaViewfromreact-nativewhen the module isn’t available).
import { useEffect, useMemo, useReducer, useState } from 'react';
import { SafeAreaView, SafeAreaProvider } from 'react-native-safe-area-context';
import {
StatusBar,
ActivityIndicator,
Linking,
Platform,
View as RNView,
StyleSheet,
useColorScheme,
} from 'react-native';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const envPort = envVariableToNumber( | ||
| process.env.STORYBOOK_WS_PORT, | ||
| websockets === 'auto' ? 7007 : (websockets?.port ?? 7007) | ||
| ); | ||
| const envSecured = envVariableToBoolean(process.env.STORYBOOK_WS_SECURED); | ||
|
|
||
| if (websockets === undefined && !envHost) { | ||
| return { | ||
| host: undefined, | ||
| port: undefined, | ||
| secured: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
loadWebsocketEnvOverrides() ignores STORYBOOK_WS_PORT/STORYBOOK_WS_SECURED when websockets is omitted and STORYBOOK_WS_HOST is not set: the early return forces port back to undefined/secured to false. This makes it impossible to override just the port (bind to all interfaces) via env vars. Consider keying the early-return on all websocket env vars (host/port/secured), or including envPort/envSecured in the returned config when provided.
| if (experimental_mcp || websockets != null || process.env.STORYBOOK_WS_HOST) { | ||
| const resolvedWs = loadWebsocketEnvOverrides(websockets); | ||
| const bindHost = | ||
| websockets === 'auto' && !process.env.STORYBOOK_WS_HOST ? undefined : resolvedWs.host; | ||
| const generateHost = | ||
| resolvedWs.host ?? | ||
| (websockets === 'auto' && !process.env.STORYBOOK_WS_HOST ? 'auto' : undefined); | ||
| const port = resolvedWs.port ?? 7007; |
There was a problem hiding this comment.
The websocket env support is gated on process.env.STORYBOOK_WS_HOST (and/or the websockets option). If a user sets only STORYBOOK_WS_PORT or STORYBOOK_WS_SECURED, this block won’t run and the overrides won’t take effect. If the intent is for all STORYBOOK_WS_* env vars to work independently, include STORYBOOK_WS_PORT/STORYBOOK_WS_SECURED in the condition(s) that decide whether to start the channel server and generate storybook.requires.
| generate({ | ||
| configPath, | ||
| useJs, | ||
| docTools, | ||
| disableUI, | ||
| ...(websocketsOption != null || process.env.STORYBOOK_WS_HOST | ||
| ? { host: generateHost, port, secured } | ||
| : {}), | ||
| }); |
There was a problem hiding this comment.
generate() only receives websocket params when websocketsOption != null || process.env.STORYBOOK_WS_HOST is true. That means STORYBOOK_WS_PORT/STORYBOOK_WS_SECURED are ignored unless the host env var is also set or websockets is explicitly configured. If env overrides are intended to work independently, consider widening this condition (or basing it on the resolved overrides) so port/secured-only env settings are reflected in the generated storybook.requires file.
| import { createServer } from 'node:http'; | ||
| import { WebSocketServer } from 'ws'; | ||
|
|
||
| const PING_INTERVAL_MS = 10_000; | ||
|
|
||
| const port = Number(process.env.STORYBOOK_WS_PORT) || 7007; | ||
| const host = process.env.STORYBOOK_WS_HOST || undefined; | ||
|
|
||
| const httpServer = createServer((_req, res) => { | ||
| res.writeHead(404); | ||
| res.end(); | ||
| }); | ||
|
|
||
| const wss = new WebSocketServer({ server: httpServer }); | ||
|
|
||
| // Same global ping interval as createChannelServer — keeps Storybook client transport alive. | ||
| const pingInterval = setInterval(() => { | ||
| wss.clients.forEach((client) => { | ||
| if (client.readyState === WebSocket.OPEN) { | ||
| client.send(JSON.stringify({ type: 'ping', args: [] })); | ||
| } |
There was a problem hiding this comment.
WebSocket.OPEN is referenced, but WebSocket is not imported/defined in this module. In Node this will throw unless a global WebSocket happens to exist. Since this script already depends on ws, import WebSocket from ws (or compare readyState against the numeric constant) to make the script reliably runnable.
dannyhw
left a comment
dannyhw
left a comment
There was a problem hiding this comment.
Found one compatibility issue worth fixing.
| setItem: async (key, value) => {}, | ||
| }; | ||
|
|
||
| const onDeviceUI = this._options.disableUI ? false : (params.onDeviceUI ?? true); |
There was a problem hiding this comment.
start still accepts options?: ReactNativeOptions, and existing or manually-authored entry files can call start({ annotations, storyEntries }) without passing options. In that case the constructor stores undefined, and getStorybookUI() throws here when it reads this._options.disableUI, so this needs a default object or optional chaining to preserve the public API.
| resolver: { | ||
| ...config.resolver, | ||
| resolveRequest: (context: any, moduleName: string, platform: string | null) => { | ||
| if (moduleName === 'tty' || moduleName === 'os') { |
There was a problem hiding this comment.
This should carry over the recent next branch fix from 95d4211 (fix: os and tty modules being emptied with Expo API Routes, #876). That change made the os/tty empty-module workaround native-only (platform !== "web") because Expo web/server bundles, including API Routes, may need the real Node built-ins. As written, the new helper empties them unconditionally, so it looks like this reintroduces the issue that #876 just fixed.
There was a problem hiding this comment.
- I believe there is still some unresolved issue regarding resolving the expo router entry point in node_modules when node_modules is not in project root (monorepo), i believe we should solve this before moving forward.
- When
enabledis explicitly false, the new@storybook/react-native/withStorybookwrapper currently returns the original Metro config, so projects that still import.rnstorybookfrom their app entry keep rendering Storybook instead of being stubbed like the existing Metro wrapper does. I think it would still be useful to stub out storybook when its disabled in order to ensure its removed from the bundle. - theres not a easy way to try it in the example app still
- lite mode should still be respected so that users can opt out of the full dependency list when using the lite-ui
- repack config should done using a plugin not with a wrapper imho
Please add something along these lines would make the Expo example easy to exercise locally:
const useNewStorybookWrapper = process.env.EXPO_PUBLIC_STORYBOOK_NEW_WRAPPER === 'true';
const { withStorybook } = useNewStorybookWrapper
? require('@storybook/react-native/withStorybook')
: require('@storybook/react-native/metro/withStorybook');{
"storybook:new-wrapper": "EXPO_PUBLIC_STORYBOOK_ENABLED=true EXPO_PUBLIC_STORYBOOK_NEW_WRAPPER=true expo start",
"storybook:new-wrapper:disabled": "EXPO_PUBLIC_STORYBOOK_NEW_WRAPPER=true expo start"
}For the implementation, the disabled Metro path could be closer to the existing metro/withStorybook behavior: if enabled was explicitly set false, wrap the Metro resolver so storybook/@storybook requests become empty modules and .rnstorybook/index.(ts|tsx|js|jsx) resolves to the stub. If there is no explicit enable signal at all, keep the current strict no-op behavior for the new wrapper.
For example, withStorybook could pass the existing enabled state into the Metro enhancer and let the enhancer apply the disabled resolver behavior when enabled === false:
const hasExplicitEnabled = process.env.STORYBOOK_ENABLED != null || options.enabled != null;
const enabled = envVariableToBoolean(process.env.STORYBOOK_ENABLED, options.enabled ?? false);
if (!enabled && !hasExplicitEnabled) {
return config;
}
if (isMetroConfig(config)) {
return enhanceMetroConfig(config, { enabled, configPath });
}Then the enhancer can branch on enabled === false while preserving its current swap behavior:
export function enhanceMetroConfig(config: MetroConfig, options: EnhanceMetroOptions = {}) {
const { swap, enabled = true, configPath } = options;
const shouldStubStorybook = enabled === false;
const normalizedConfigPath = configPath ? path.resolve(configPath) : undefined;
const stubPath = path.resolve(
__dirname,
__dirname.includes(path.sep + 'src') ? './stub.tsx' : './stub.js'
);
return {
...config,
resolver: {
...config.resolver,
resolveRequest: (context, moduleName, platform) => {
const resolveFunction = config.resolver?.resolveRequest ?? context.resolveRequest;
if (shouldStubStorybook && (moduleName.startsWith('storybook') || moduleName.startsWith('@storybook'))) {
return { type: 'empty' };
}
if (platform !== 'web' && (moduleName === 'tty' || moduleName === 'os')) {
return { type: 'empty' };
}
const resolved = resolveFunction(context, moduleName, platform);
if (shouldStubStorybook && resolved.filePath && normalizedConfigPath) {
const resolvedFilePath = path.resolve(resolved.filePath);
const isConfigIndex = ['index.ts', 'index.tsx', 'index.js', 'index.jsx'].some(
(file) => resolvedFilePath === path.join(normalizedConfigPath, file)
);
if (isConfigIndex) {
return { filePath: stubPath, type: 'sourceFile' };
}
if (resolvedFilePath.startsWith(normalizedConfigPath + path.sep)) {
return { type: 'empty' };
}
}
return resolved;
},
},
};
}
| }; | ||
| } | ||
|
|
||
| export function enhanceRepackConfig<T extends Record<string, any>>( |
There was a problem hiding this comment.
I don't think this is the expected way to handle extending config of rspack/repack
also config in repack can be a sync or async function as well as a object so this would break in some cases
|
after thinking it through I'm not sure a unified wrapper is the right direction since repack is quite different, repack builds on webpack/rspack and uses plugins for extended behaviour, we only wrap metro config because of its lacking support for plugins or other ways to extend metro behaviour. it would likely be better to just extract reusable code and provide a repack plugin like we currently do but updated for the expected new flow (or exclude it entirely from this initial version) |
Tracked by storybookjs/storybook#34276 ("Easy React Native Setup")
Companion PR: storybookjs/storybook#34333 (CLI init overhaul, codemods, automigrations)
Closes #873 (
deviceAddons)Documentation PR: storybookjs/react-native#877
What this does
This PR adds a bundler-agnostic
withStorybookwrapper at@storybook/react-native/withStorybookthat introduces entry-point swapping as the new recommended way to run Storybook for React Native — matching the mental model of Storybook for Web, where Storybook runs as its own entry point rather than being deeply integrated into the user's app.It also introduces the
deviceAddonsconfig property, separating on-device addons from theaddonsarray to preventCriticalPresetLoadErrorduring server-side operations likeextract.Entry-point swapping
The new
withStorybookwrapper:.rnstorybook/indexwhenSTORYBOOK_ENABLED=trueis set — no changes toApp.tsxrequiredSTORYBOOK_ENABLEDis not set — zero Storybook code in the production bundleSTORYBOOK_ENABLED,STORYBOOK_WS_HOST,STORYBOOK_WS_PORT,STORYBOOK_WS_SECURED,STORYBOOK_SERVER,STORYBOOK_DISABLE_UI), so the same metro config works for all environmentsstorybook.requiresfile, eliminating the need to manually match config betweenwithStorybookandgetStorybookUIThe existing
./metro/withStorybookand./repack/withStorybookexports are completely untouched and remain fully supported for projects using the in-app integration approach.deviceAddonsOn-device addons (like
@storybook/addon-ondevice-controls) contain React Native code that can't be evaluated in Node.js. When listed inaddons, Storybook Core tries to load them as presets during operations likeextract, which fails. The newdeviceAddonsproperty ensures they're only loaded at runtime on the device.Backwards compatible —
addonsstill works and a console warning guides users to migrate. The companion PR (storybookjs/storybook#34333) includes an automigration (rn-ondevice-addons-to-device-addons) that handles this automatically vianpx storybook automigrate.How it works together
This PR (react-native repo) provides the runtime bundler wrapper. The companion PR (storybook repo) provides the CLI tooling:
Concern | This PR | storybookjs/storybook#34333 -- | -- | -- Bundler-agnostic withStorybook | ✅ Implementation | — Entry-point swapping | ✅ Metro resolver + Re.Pack plugin | — Env var support | ✅ Runtime reading | — deviceAddons type + runtime | ✅ StorybookConfig type, generate.js | — storybook init for RN | — | ✅ Codemod, entrypoint generation, scripts deviceAddons automigration | — | ✅ rn-ondevice-addons-to-device-addons Metro config codemod | — | ✅ AST-based withStorybook() wrappingManual verification
Existing
withStorybook(backwards compatibility)Upgraded two existing projects to the canary release of this branch to verify the legacy
./metro/withStorybookpath is unaffected:Both projects continued to function correctly with their existing metro config and in-app Storybook integration, confirming zero regressions.
storybook initCLITested
npx storybook@0.0.0-pr-34333-sha-865b50bc initin a fresh Expo project:.rnstorybook/index.tsgenerated correctlymetro.config.jswrapped withwithStorybook()using the bundler-agnostic importstorybook:iosandstorybook:androidscripts added topackage.jsonmain.ts(and other templates) from thelatestpublished version of@storybook/react-native, not from the canary. This means the generatedmain.tswon't havedeviceAddonsuntil this PR is released. This will resolve itself once we publish the minor release.npx storybook automigrateVerified that
npx storybook@0.0.0-pr-34333-sha-865b50bc automigratecorrectly moves on-device addons fromaddons→deviceAddonsin an existing project's.rnstorybook/main.ts.Release plan
Targeting release as a new minor version this week (v10.4), alongside:
entry-point-swapping-docsbranch — covers updated getting started guides, migration guide, environment variables reference, and README rewrite)Test coverage
@storybook/react-native(unit tests forwithStorybook,enhanceMetroConfig,enhanceRepackConfig, entry-point resolution, env var handling)testspackage (story generation snapshots)