Skip to content
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

chore: Reduce hydration comment for {:else if} #15250

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ export function Fragment(node, context) {
const is_single_element = trimmed.length === 1 && trimmed[0].type === 'RegularElement';
const is_single_child_not_needing_template =
trimmed.length === 1 &&
(trimmed[0].type === 'SvelteFragment' || trimmed[0].type === 'TitleElement');
(trimmed[0].type === 'SvelteFragment' ||
trimmed[0].type === 'TitleElement' ||
(trimmed[0].type === 'IfBlock' && trimmed[0].elseif));

const template_name = context.state.scope.root.unique('root'); // TODO infer name from parent

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/** @import { BlockStatement, Expression } from 'estree' */
/** @import { BlockStatement, Expression, Identifier } from 'estree' */
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types' */
import * as b from '../../../../utils/builders.js';
Expand All @@ -19,28 +19,29 @@ export function IfBlock(node, context) {
let alternate_id;

if (node.alternate) {
const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
alternate_id = context.state.scope.generate('alternate');
statements.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate)));
const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
const nodes = node.alternate.nodes;

let alternate_args = [b.id('$$anchor')];
if (nodes.length === 1 && nodes[0].type === 'IfBlock' && nodes[0].elseif) {
alternate_args.push(b.id('$$elseif'));
}

statements.push(b.var(b.id(alternate_id), b.arrow(alternate_args, alternate)));
}

/** @type {Expression[]} */
const args = [
context.state.node,
node.elseif ? b.id('$$anchor') : context.state.node,
b.arrow(
[b.id('$$render')],
b.block([
b.if(
/** @type {Expression} */ (context.visit(node.test)),
b.stmt(b.call(b.id('$$render'), b.id(consequent_id))),
alternate_id
? b.stmt(
b.call(
b.id('$$render'),
b.id(alternate_id),
node.alternate ? b.literal(false) : undefined
)
)
? b.stmt(b.call(b.id('$$render'), b.id(alternate_id), b.literal(false)))
: undefined
)
])
Expand Down Expand Up @@ -69,10 +70,10 @@ export function IfBlock(node, context) {
// ...even though they're logically equivalent. In the first case, the
// transition will only play when `y` changes, but in the second it
// should play when `x` or `y` change — both are considered 'local'
args.push(b.literal(true));
args.push(b.id('$$elseif'));
}

statements.push(b.stmt(b.call('$.if', ...args)));

context.state.init.push(b.block(statements));
context.state.init.push(...statements);
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/** @import { BlockStatement, Expression } from 'estree' */
/** @import { BlockStatement, Expression, IfStatement } from 'estree' */
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types.js' */
import { BLOCK_OPEN_ELSE } from '../../../../../internal/server/hydration.js';
Expand All @@ -10,19 +10,29 @@ import { block_close, block_open } from './shared/utils.js';
* @param {ComponentContext} context
*/
export function IfBlock(node, context) {
const test = /** @type {Expression} */ (context.visit(node.test));

const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));
consequent.body.unshift(b.stmt(b.assignment('+=', b.id('$$payload.out'), block_open)));
let if_statement = b.if(/** @type {Expression} */ (context.visit(node.test)), consequent);

const alternate = node.alternate
? /** @type {BlockStatement} */ (context.visit(node.alternate))
: b.block([]);
context.state.template.push(if_statement, block_close);

consequent.body.unshift(b.stmt(b.assignment('+=', b.id('$$payload.out'), block_open)));
let index = 1;
let alt = node.alternate;
while (alt && alt.nodes.length === 1 && alt.nodes[0].type === 'IfBlock' && alt.nodes[0].elseif) {
const elseif = alt.nodes[0];
const alternate = /** @type {BlockStatement} */ (context.visit(elseif.consequent));
alternate.body.unshift(
b.stmt(b.assignment('+=', b.id('$$payload.out'), b.literal(`<!--[${index++}-->`)))
);
if_statement = if_statement.alternate = b.if(
/** @type {Expression} */ (context.visit(elseif.test)),
alternate
);
alt = elseif.alternate;
}

alternate.body.unshift(
if_statement.alternate = alt ? /** @type {BlockStatement} */ (context.visit(alt)) : b.block([]);
if_statement.alternate.body.unshift(
b.stmt(b.assignment('+=', b.id('$$payload.out'), b.literal(BLOCK_OPEN_ELSE)))
);

context.state.template.push(b.if(test, consequent, alternate), block_close);
}
38 changes: 26 additions & 12 deletions packages/svelte/src/internal/client/dom/blocks/if.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ import {
set_hydrating
} from '../hydration.js';
import { block, branch, pause_effect, resume_effect } from '../../reactivity/effects.js';
import { HYDRATION_START_ELSE, UNINITIALIZED } from '../../../../constants.js';
import { HYDRATION_START, HYDRATION_START_ELSE, UNINITIALIZED } from '../../../../constants.js';

/**
* @param {TemplateNode} node
* @param {(branch: (fn: (anchor: Node) => void, flag?: boolean) => void) => void} fn
* @param {boolean} [elseif] True if this is an `{:else if ...}` block rather than an `{#if ...}`, as that affects which transitions are considered 'local'
* @param {(branch: (fn: (anchor: Node, elseif?: [number,number]) => void, flag?: boolean) => void) => void} fn
* @param {[number,number]} [elseif]
* @returns {void}
*/
export function if_block(node, fn, elseif = false) {
if (hydrating) {
export function if_block(node, fn, [root_index, hydrate_index] = [0, 0]) {
if (hydrating && root_index === 0) {
hydrate_next();
}

Expand All @@ -33,35 +33,49 @@ export function if_block(node, fn, elseif = false) {
/** @type {UNINITIALIZED | boolean | null} */
var condition = UNINITIALIZED;

var flags = elseif ? EFFECT_TRANSPARENT : 0;
var flags = root_index > 0 ? EFFECT_TRANSPARENT : 0;

var has_branch = false;

const set_branch = (/** @type {(anchor: Node) => void} */ fn, flag = true) => {
const set_branch = (
/** @type {(anchor: Node, elseif?: [number,number]) => void} */ fn,
flag = true
) => {
has_branch = true;
update_branch(flag, fn);
};

const update_branch = (
/** @type {boolean | null} */ new_condition,
/** @type {null | ((anchor: Node) => void)} */ fn
/** @type {null | ((anchor: Node, elseif?: [number,number]) => void)} */ fn
) => {
if (condition === (condition = new_condition)) return;

/** Whether or not there was a hydration mismatch. Needs to be a `let` or else it isn't treeshaken out */
let mismatch = false;

if (hydrating) {
const is_else = /** @type {Comment} */ (anchor).data === HYDRATION_START_ELSE;
if (hydrating && hydrate_index !== -1) {
if (root_index === 0) {
const data = /** @type {Comment} */ (anchor).data;
if (data === HYDRATION_START) {
hydrate_index = 0;
} else if (data === HYDRATION_START_ELSE) {
hydrate_index = Infinity;
} else {
hydrate_index = parseInt(data.substring(1));
}
}
const is_else = hydrate_index > root_index;

if (!!condition === is_else) {
if (!!condition === is_else || isNaN(hydrate_index)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely don't want to be using isNaN anywhere, it's a major codesmell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't known that.
What the alternative to detect bad hydration comment ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validate it before doing parseInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about hydrate_index !== hydrate_index, which will be true only for NaN ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean you also have to facilitate mismatches, where the SSR content is intentionally different from the client side logic too, right? So in that case wouldn't the indexes also not match up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes in this case the indexes will not match.

Here it is rather the case where the comment is not from the Svelte hydration process...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't fully understand what the problem here is - isn't this more straightforward/less code to just go "this isn't a number, so something else went really wrong"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNaN has been condemned to hell all over, you can research its history. I'd just rather avoid it and it's caveats where possible.

// Hydration mismatch: remove everything inside the anchor and start fresh.
// This could happen with `{#if browser}...{/if}`, for example
anchor = remove_nodes();

set_hydrate_node(anchor);
set_hydrating(false);
mismatch = true;
hydrate_index = -1; // ignore hydration in next else if
}
}

Expand All @@ -81,7 +95,7 @@ export function if_block(node, fn, elseif = false) {
if (alternate_effect) {
resume_effect(alternate_effect);
} else if (fn) {
alternate_effect = branch(() => fn(anchor));
alternate_effect = branch(() => fn(anchor, [root_index + 1, hydrate_index]));
}

if (consequent_effect) {
Expand Down