Adjust boundary error message #16748
Conversation
…undary-error-message
|
| ``` | ||
|
|
||
| The `await` keyword can only appear in a `$derived(...)` or template expression, or at the top level of a component's `<script>` block, if it is inside a [`<svelte:boundary>`](/docs/svelte/svelte-boundary) that has a `pending` snippet: | ||
| The `await` keyword can only appear in a `$derived(...)` or template expression, or at the top level of a component's `<script>` block, if it is inside a [`<svelte:boundary>`](/docs/svelte/svelte-boundary): |
There was a problem hiding this comment.
TODO: Feels like this error should go away in favor of an internal invariant. Because we wrap your app in a root boundary it should never be possible to encounter.
| @@ -177,7 +177,8 @@ export default function element(parser) { | |||
| mathml: false, | |||
| scoped: false, | |||
| has_spread: false, | |||
| path: [] | |||
| path: [], | |||
| synthetic_value_node: null | |||
There was a problem hiding this comment.
This is part of a fix to options
| @@ -11,7 +11,9 @@ export function create_fragment(transparent = false) { | |||
| metadata: { | |||
| transparent, | |||
| dynamic: false, | |||
| has_await: false | |||
| has_await: false, | |||
There was a problem hiding this comment.
we were forgetting to initialize this previously, which luckily wasn't a problem because undefined is falsey
| has_await: false | ||
| has_await: false, | ||
| // name is added later, after we've done scope analysis | ||
| hoisted_promises: { name: '', promises: [] } |
There was a problem hiding this comment.
Happy to see a better solution for this (see below in `2-analyze/index.js)
| @@ -180,6 +169,54 @@ export class Boundary { | |||
| } | |||
| } | |||
|
|
|||
| #detect_server_state() { | |||
There was a problem hiding this comment.
TODO this is a weak name
| * @returns {Node | null} | ||
| */ | ||
| export function first_child(fragment, is_text) { | ||
| export function first_child(fragment, is_text = false) { |
There was a problem hiding this comment.
this was being called without a second argument in compiled code so I updated it just to make it more correct
| var ctx = /** @type {ComponentContext} */ (component_context); | ||
| ctx.c = context; | ||
| } | ||
| boundary( |
There was a problem hiding this comment.
note for reviewers: in reality this is very similar to what we had before with branch, as children is just stuck into a branch inside of boundary. The only real difference is that the branch is now inside of a block.
There was a problem hiding this comment.
As Rich pointed out, Payload is kind of a weird name now that this is a Big Thing with logic complicatedness and stuff. Should it be called ContentTree or something now?
| @@ -31,8 +31,21 @@ export function asClassComponent(component) { | |||
| html: result.body | |||
| }; | |||
| }; | |||
| /** @type {(props?: {}, opts?: { $$slots?: {}; context?: Map<any, any>; }) => Promise<{ html: any; css: { code: string; map: any; }; head: string; }> } */ | |||
There was a problem hiding this comment.
unsure if we should do this -- I needed it to be able to test with minimal changes to Kit but if we're dropping svelte 4 in the next major it could be worth just not supporting async rendering for class components. that being said it's a tiny lift.
| if (context.state.async_hoist_boundary) { | ||
| const len = context.state.async_hoist_boundary.metadata.hoisted_promises.promises.push( | ||
| node.argument | ||
| ); | ||
| context.state.analysis.hoisted_promises.set( | ||
| node.argument, | ||
| b.member( | ||
| b.id(context.state.async_hoist_boundary.metadata.hoisted_promises.name), | ||
| b.literal(len - 1), | ||
| true | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
this logic isn't quite right — if you have something like this...
{await (await duplicated())}...you get this output:
$$payload.child(async ($$payload) => {
const promises = [await duplicated(), duplicated()];
$$payload.child(async ($$payload) => {
$$payload.push(`<!---->${$.escape(await promises[0])}`);
});
});There was a problem hiding this comment.
(semi-related: it'd be nice if we skipped the indirection in the common case that there's only a single promise)
There was a problem hiding this comment.
the latter issue is resolved on remove-async-hoist-boundary
There was a problem hiding this comment.
Actually you know what? I think this is the wrong place to think about hoisting. It means that in a case like this...
{false && await foo()}...foo() will be called, incorrectly. Or even worse:
{object && await object.promise}I suspect we need to use the same de-waterfalling mechanism that's used for client code
There was a problem hiding this comment.
(well not the same, since Memoizer is concerned with... memoizing, but you get the idea)
| const promises = [Promise.resolve([first, second, third])]; | ||
| const each_array = $.ensure_array_like(await promises[0]); | ||
|
|
||
| $$payload.child(async ($$payload) => { |
There was a problem hiding this comment.
why is this an async function? Nothing is awaited inside it. I'd expect either this to be a sync function, or to see this...
const each_array = $.ensure_array_like(await Promise.resolve([first, second, third]));...inside this call (in which case the topmost $$payload.child(...) would be passed a sync function)
| const { promise: main_promise, resolve: main_resolve } = Promise.withResolvers(); | ||
| const { promise: a_promise, resolve: a_resolve } = Promise.withResolvers(); | ||
| const { promise: b_promise, resolve: b_resolve } = Promise.withResolvers(); |
There was a problem hiding this comment.
minor detail but it's way easier to do it like this :)
| const { promise: main_promise, resolve: main_resolve } = Promise.withResolvers(); | |
| const { promise: a_promise, resolve: a_resolve } = Promise.withResolvers(); | |
| const { promise: b_promise, resolve: b_resolve } = Promise.withResolvers(); | |
| const main = Promise.withResolvers(); | |
| const a = Promise.withResolvers(); | |
| const b = Promise.withResolvers(); |
There was a problem hiding this comment.
also noted, will clean up on test run thru
|
|
||
| // regardless of resolution order, title should be the result of B, because it's the last-encountered | ||
| tick().then(() => { | ||
| main_resolve(true); |
There was a problem hiding this comment.
| main_resolve(true); | |
| main.resolve(true); |
| tick().then(() => { | ||
| main_resolve(true); | ||
| tick().then(() => { | ||
| b_resolve(true); |
There was a problem hiding this comment.
| b_resolve(true); | |
| b.resolve(true); |
| tick().then(() => { | ||
| b_resolve(true); | ||
| }).then(() => { | ||
| a_resolve(true); |
There was a problem hiding this comment.
| a_resolve(true); | |
| a.resolve(true); |
| </script> | ||
|
|
||
| <svelte:head> | ||
| {#if await main_promise} |
There was a problem hiding this comment.
| {#if await main_promise} | |
| {#if await main.promise} |
| <A promise={a_promise}/> | ||
| <B promise={b_promise}/> |
There was a problem hiding this comment.
| <A promise={a_promise}/> | |
| <B promise={b_promise}/> | |
| <A promise={a.promise}/> | |
| <B promise={b.promise}/> |
| if (node.type === 'Fragment') { | ||
| node.metadata.hoisted_promises.name = state.scope.generate('promises'); | ||
| } |
There was a problem hiding this comment.
any particular reason this is here instead of the Fragment visitor?
There was a problem hiding this comment.
because I felt like it 😂
no really it's here because I happened to be in this file looking at this function when I was thinking about it. Will move
There was a problem hiding this comment.
adjusted this on the remove-async-hoist-boundary branch
| /** | ||
| * The "anchor" fragment for any hoisted promises. This is the root fragment when | ||
| * walking starts and until another boundary fragment is encountered, like a | ||
| * consequent or alternate of an `#if` or `#each` block. When this fragment is emitted | ||
| * during server transformation, the promise expressions will be hoisted out of the fragment | ||
| * and placed right above it in an array. | ||
| */ | ||
| async_hoist_boundary: AST.Fragment | null; |
There was a problem hiding this comment.
this is unnecessary, we can just re-use fragment. See remove-async-hoist-boundary
* use fragment as async hoist boundary * remove async_hoist_boundary * only dewaterfall when necessary * unused * simplify/fix * de-waterfall awaits in separate elements * update snapshots * remove unnecessary wrapper * fix * fix * remove suspends_without_fallback --------- Co-authored-by: Rich Harris <[email protected]>
| /** | ||
| * @param {(value: { head: string, body: string }) => void} onfulfilled | ||
| */ | ||
| async then(onfulfilled) { | ||
| const content = await Payload.#collect_content([this], this.type); | ||
| return onfulfilled(content); | ||
| } |
There was a problem hiding this comment.
what's this for? Find All References doesn't show anything — is there an await $$payload somewhere? could use a comment if so
There was a problem hiding this comment.
It's awaited in render_async. This could be a method like collect_async too 🤷
Co-authored-by: Rich Harris <[email protected]>
…velte into adjust-boundary-error-message
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint