-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
SvelteKit: Fix set_context_after_init error when experimental async is enabled
#32513
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
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
JReinhold
left a 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.
This is great! I'm going to make some modifications to it to make it a bit more resilient, but it's a great starting point. thank you 🙏
WalkthroughAdds a MockProvider Svelte component and routes the SvelteKit preview decorator to render it; exposes the component via build-config and package exports; updates tsconfig; and extends sandbox-init to prepare Svelte projects (enable async Svelte and remote functions) and adjust framework detection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SB as Storybook Preview
participant Decorator as svelteKitMocksDecorator
participant MP as MockProvider (Svelte)
participant Win as window/document
SB->>Decorator: Render story
Decorator-->>SB: { Component: MockProvider, props: { svelteKitParameters } }
SB->>MP: Mount with props
MP->>Win: Register click + SB event listeners
Note over MP,Win: Listeners: storybook:goto, invalidate, invalidateAll, pushState, replaceState, enhance
User->>Win: Click <a> link
Win->>MP: Dispatch click
alt href match (exact/regex)
MP->>SB: Navigate action or user callback
else no match
MP-->>User: Allow default
end
SB-->>MP: Unmount
MP->>Win: Cleanup listeners
sequenceDiagram
autonumber
participant Init as Sandbox init
participant Detect as Framework detector
participant Prep as prepareSvelteSandbox
participant FS as svelte.config.(js|ts)
Init->>Detect: Identify renderer
alt @storybook/svelte
Init->>Prep: prepareSvelteSandbox(cwd)
Prep->>FS: Read config
Prep->>FS: Enable experimental.async + kit.experimental.remoteFunctions
Prep->>FS: Write updated config
else other renderers
Detect-->>Init: Continue default flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
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: 0
🧹 Nitpick comments (4)
code/frameworks/sveltekit/static/MockProvider.svelte (3)
5-6: Import from package exports, not../src/*Importing TS sources from a published static Svelte file can force consumers to compile your TS sources. Prefer the public export paths so consumers get the built JS.
Apply:
-import { setAfterNavigateArgument } from '../src/mocks/app/navigation'; -import { setNavigating, setPage, setUpdated } from '../src/mocks/app/stores'; +import { setAfterNavigateArgument } from '@storybook/sveltekit/internal/mocks/app/navigation'; +import { setNavigating, setPage, setUpdated } from '@storybook/sveltekit/internal/mocks/app/stores';
25-60: Harden link interception: respect modifier keys/targets and avoidfindLast
- Don’t intercept when users use modifier keys (cmd/ctrl/shift/alt), non-left clicks,
target="_blank", ordownloadlinks.Array.prototype.findLastis newer; prefer a compatible approach.- const globalClickListener = (e) => { - // we add a global click event listener and we check if there's a link in the composedPath - const path = e.composedPath(); - const element = path.findLast((el) => el instanceof HTMLElement && el.tagName === 'A'); - if (element && element instanceof HTMLAnchorElement) { + const globalClickListener = (e) => { + if (e.defaultPrevented || e.button !== 0) return; // only unmodified left-clicks + const path = e.composedPath(); + // find first anchor in the composed path (compatible with older browsers) + const element = path.find((el) => el instanceof HTMLAnchorElement); + if (element) { // if the element is an a-tag we get the href of the element // and compare it to the hrefs-parameter set by the user const to = element.getAttribute('href'); if (!to) { return; } + // respect typical browser behaviors for modified/external clicks + if ( + element.target === '_blank' || + element.hasAttribute('download') || + e.metaKey || e.ctrlKey || e.shiftKey || e.altKey + ) { + return; + } e.preventDefault(); const defaultActionCallback = () => action('navigate')(to, e); if (!svelteKitParameters.hrefs) { defaultActionCallback(); return; }
72-111: Fix JSDoc param description fordefaultToActionThe description repeats “The list of functions…”. Make it clear it toggles fallback to actions when a handler isn’t provided.
- * @param {boolean} [defaultToAction] The list of functions in that module that emit events + * @param {boolean} [defaultToAction] If true, attach listeners that fall back to actions when no handler is provided.scripts/tasks/sandbox-parts.ts (1)
890-919: Make prepareSvelteSandbox resilient and idempotent (avoid failing on TS “satisfies”, skip no-op writes)csfReadConfig can fail parsing svelte.config.ts (e.g. TS
satisfies); avoid throwing and skip writes when no changes are needed.async function prepareSvelteSandbox(cwd: string) { const svelteConfigJsPath = join(cwd, 'svelte.config.js'); const svelteConfigTsPath = join(cwd, 'svelte.config.ts'); // Check which config file exists const configPath = (await pathExists(svelteConfigTsPath)) ? svelteConfigTsPath : (await pathExists(svelteConfigJsPath)) ? svelteConfigJsPath : null; if (!configPath) { throw new Error( `No svelte.config.js or svelte.config.ts found in sandbox: ${cwd}, cannot modify config.` ); } - const svelteConfig = await csfReadConfig(configPath); + let svelteConfig: ConfigFile; + try { + svelteConfig = await csfReadConfig(configPath); + } catch (e: any) { + logger.warn( + `Skipping Svelte config tweaks; unable to parse ${relative(cwd, configPath)}: ${e?.message ?? e}` + ); + return; + } // Enable async components - // see https://svelte.dev/docs/svelte/await-expressions - svelteConfig.setFieldValue(['compilerOptions', 'experimental', 'async'], true); + // Experimental async Svelte compiler option + const prevAsync = + !!svelteConfig.getFieldValue(['compilerOptions', 'experimental', 'async']); + if (!prevAsync) { + svelteConfig.setFieldValue(['compilerOptions', 'experimental', 'async'], true); + } // Enable remote functions // see https://svelte.dev/docs/kit/remote-functions - svelteConfig.setFieldValue(['kit', 'experimental', 'remoteFunctions'], true); + const prevRemoteFns = + !!svelteConfig.getFieldValue(['kit', 'experimental', 'remoteFunctions']); + if (!prevRemoteFns) { + svelteConfig.setFieldValue(['kit', 'experimental', 'remoteFunctions'], true); + } - - await writeConfig(svelteConfig); + if (!prevAsync || !prevRemoteFns) { + await writeConfig(svelteConfig); + } else { + logger.info('Svelte config already enables experimental.async and kit.experimental.remoteFunctions'); + } }Update the async comment link to: https://svelte.dev/docs/svelte-compiler (documents compilerOptions.experimental.async).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/frameworks/sveltekit/build-config.ts(1 hunks)code/frameworks/sveltekit/package.json(2 hunks)code/frameworks/sveltekit/src/preview.ts(1 hunks)code/frameworks/sveltekit/static/MockProvider.svelte(1 hunks)code/frameworks/sveltekit/tsconfig.json(0 hunks)scripts/tasks/sandbox-parts.ts(3 hunks)
💤 Files with no reviewable changes (1)
- code/frameworks/sveltekit/tsconfig.json
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/frameworks/sveltekit/static/MockProvider.sveltecode/frameworks/sveltekit/package.jsoncode/frameworks/sveltekit/src/preview.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with vi.mocked()
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to type and access mocked functions
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/frameworks/sveltekit/package.json
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/frameworks/sveltekit/package.json
🧬 Code graph analysis (2)
code/frameworks/sveltekit/src/preview.ts (1)
code/frameworks/sveltekit/src/types.ts (1)
SvelteKitParameters(57-73)
scripts/tasks/sandbox-parts.ts (1)
code/core/src/csf-tools/ConfigFile.ts (1)
writeConfig(1180-1187)
⏰ 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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (11)
code/frameworks/sveltekit/package.json (2)
50-50: Includingstatic/**/*looks goodThis ensures the new
MockProvider.svelteis published. No concerns.
35-36: Verify Vite/Svelte resolves @storybook/sveltekit/internal/MockProvider.svelte at runtimepackage.json exports "./internal/MockProvider.svelte" → "./static/MockProvider.svelte"; preview.ts imports "@storybook/sveltekit/internal/MockProvider.svelte"; static file exists at code/frameworks/sveltekit/static/MockProvider.svelte. Run Storybook dev and build-preview (builder-vite and any other supported builders) and confirm there are no resolver errors and MockProvider is loaded at runtime.
code/frameworks/sveltekit/static/MockProvider.svelte (3)
12-17: Sets context before children init — this fixes async Svelte timingThis is the right approach to satisfy Svelte async’s stricter getContext/setContext timing.
129-129: Children render looks good
{@render children()}is appropriate here.
3-3: 'storybook/actions' is correct — no change required. Repo-wide usage and internal aliases expose that entrypoint from core (see code/build/utils/generate-bundle.ts and code/core/src/preview/globals/runtime.ts).code/frameworks/sveltekit/src/preview.ts (2)
9-14: Decorator returns a component wrapper — the right shape for SvelteThis moves side effects into a component so contexts are set before story init. Good change.
2-2: Approve — MockProvider import and decorator return are correctpreview.ts returns { Component: MockProvider, props: { svelteKitParameters } }, matching other render/decorator examples found in the repo; no changes required.
code/frameworks/sveltekit/build-config.ts (1)
48-50: extraOutputs is supported — no change requiredBuildEntries includes extraOutputs (scripts/build/utils/entry-utils.ts) and scripts/build/utils/generate-package-json.ts spreads data.extraOutputs into package.json.exports, so the './internal/MockProvider.svelte' → './static/MockProvider.svelte' mapping will be included in the built package exports.
scripts/tasks/sandbox-parts.ts (3)
20-24: Good import alias to avoid name collision with local readConfigAliasing csf-tools' readConfig prevents confusion with utils/main-js' readConfig.
192-192: Correct type for react-native-webSetting extra.type = 'react_native_web' here is appropriate.
177-188: Prep Svelte sandbox before sb init — SvelteKit templates coveredprepareSvelteSandbox is called when template.expected.renderer === '@storybook/svelte' (scripts/tasks/sandbox-parts.ts), and code/lib/cli-storybook/src/sandbox-templates.ts defines the SvelteKit template ('svelte-kit/skeleton-ts') with renderer '@storybook/svelte', so SvelteKit templates will hit this branch.
|
View your CI Pipeline Execution ↗ for commit 41b0ff0
☁️ Nx Cloud last updated this comment at |
set_context_after_init error when experimental async is enabled
JReinhold
left a 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.
Awesome job, thank you!
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: 4
🧹 Nitpick comments (1)
code/frameworks/sveltekit/static/MockProvider.svelte (1)
61-73: Docstring fix and default parameterClarify defaultToAction semantics and give it a default.
Apply this diff:
- * @param {boolean} [defaultToAction] The list of functions in that module that emit events + * @param {boolean} [defaultToAction=false] If true, fall back to action(func) when no user callback is provided- function createListeners(baseModule, functions, defaultToAction) { + function createListeners(baseModule, functions, defaultToAction = false) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/frameworks/sveltekit/package.json(2 hunks)code/frameworks/sveltekit/static/MockProvider.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/sveltekit/package.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/frameworks/sveltekit/static/MockProvider.svelte
🔇 Additional comments (4)
code/frameworks/sveltekit/static/MockProvider.svelte (4)
12-17: Good: Context set before child initMoving setPage/setNavigating/setUpdated/setAfterNavigateArgument to component init resolves set_context_after_init under async Svelte.
Ensure these setters are idempotent per mount and don’t leak across stories.
113-119: LGTM: Event listener setupListener matrix for navigation/forms looks correct; cleanup provided below.
2-3: Verify import path for actionsConfirm 'storybook/actions' resolves in this package build; many examples use '@storybook/addon-actions'.
If needed, switch to:
- import { action } from 'storybook/actions'; + import { action } from '@storybook/addon-actions';
1-1: Name vs PR textPR text mentions MockContext.svelte; file is MockProvider.svelte. Align names or update description to avoid confusion.
| const normalizeHrefConfig = (hrefConfig) => { | ||
| if (typeof hrefConfig === 'function') { | ||
| return { callback: hrefConfig, asRegex: false }; | ||
| } | ||
| return hrefConfig; | ||
| }; |
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.
🧹 Nitpick
Normalize href config defensively
If a non‑object slips in, return a safe default.
Apply this diff:
- const normalizeHrefConfig = (hrefConfig) => {
- if (typeof hrefConfig === 'function') {
- return { callback: hrefConfig, asRegex: false };
- }
- return hrefConfig;
- };
+ const normalizeHrefConfig = (hrefConfig) => {
+ if (typeof hrefConfig === 'function') return { callback: hrefConfig, asRegex: false };
+ return hrefConfig && typeof hrefConfig === 'object'
+ ? hrefConfig
+ : { callback: undefined, asRegex: false };
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalizeHrefConfig = (hrefConfig) => { | |
| if (typeof hrefConfig === 'function') { | |
| return { callback: hrefConfig, asRegex: false }; | |
| } | |
| return hrefConfig; | |
| }; | |
| const normalizeHrefConfig = (hrefConfig) => { | |
| if (typeof hrefConfig === 'function') return { callback: hrefConfig, asRegex: false }; | |
| return hrefConfig && typeof hrefConfig === 'object' | |
| ? hrefConfig | |
| : { callback: undefined, asRegex: false }; | |
| }; |
🤖 Prompt for AI Agents
In code/frameworks/sveltekit/static/MockProvider.svelte around lines 18 to 23,
normalizeHrefConfig should defensively handle non-object inputs: keep the
existing branch for functions, but if hrefConfig is not a non-null object return
a safe default like { callback: (href) => href, asRegex: false }; if it is an
object ensure it has a callback (defaulting to identity) and an asRegex boolean
(defaulting to false) before returning it.
|
Unfortunately due to the heavy changes we've made in Storybook 10 (current |
|
Thanks @JReinhold - do you have an eta on the v10 beta release? This fix is eagerly awaited. |
|
@magick93 should go out today or tomorrow. |
Closes #32281
What I did
When setting the flag for experimental async support, usage of
getContext/setContextis more strictly enforced by svelte, this breaks the way that the sveltekit framework mocked built-in sveltekit contexts.The current approach of calling
setContextin the decorator function body, happens after component initialization, which is not allowed.Wrapping the Story in a context provider for mocking sveltekit stores means we can mock the contexts at the right time (before component initialization).
Most of the code that was in the
preview.tsis now inMockContext.svelteTesting
The changes in this PR are covered in the following automated tests:
There are some stories already that check if context mocking works, these still pass.
Manual testing
I tested against the repo in the github issue https://github.com/jhechtf/storybook-svelte-5-repro, and all works fine, including actually using the new async functionality.
Also checking out the
svelte-kit/skeleton-tssandbox and running tests, everything passing.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.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor
Chores