Skip to content

Commit 0e8f49b

Browse files
authored
chore: rebase batches after process, not during (#17900)
This is part of me trying to figure out #17162. It feels less confusing to rebase other branches after the current batch has been processed, rather than sort of doing it in the middle (which is an artifact of historical constraints that no longer apply). No test because it doesn't change any user-observable behaviour (but I added a changeset just in case) ### Before submitting the PR, please make sure you do the following - [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs - [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`. - [x] This message body should clearly illustrate what problems it solves. - [ ] Ideally, include a test that fails without this PR but passes with it. - [x] If this PR changes code within `packages/svelte/src`, add a changeset (`npx changeset`). ### Tests and linting - [x] Run the tests with `pnpm test` and lint the project with `pnpm lint`
1 parent 72cd247 commit 0e8f49b

2 files changed

Lines changed: 57 additions & 60 deletions

File tree

.changeset/upset-parts-throw.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
chore: rebase batches after process, not during

packages/svelte/src/internal/client/reactivity/batch.js

Lines changed: 52 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,10 @@ export class Batch {
249249
reset_branch(e, t);
250250
}
251251
} else {
252+
if (this.#pending === 0) {
253+
batches.delete(this);
254+
}
255+
252256
// clear effects. Those that are still needed will be rescheduled through unskipping the skipped branches.
253257
this.#dirty_effects.clear();
254258
this.#maybe_dirty_effects.clear();
@@ -262,10 +266,6 @@ export class Batch {
262266
flush_queued_effects(effects);
263267
previous_batch = null;
264268

265-
if (this.#pending === 0) {
266-
this.#commit();
267-
}
268-
269269
this.#deferred?.resolve();
270270
}
271271

@@ -290,6 +290,10 @@ export class Batch {
290290

291291
next_batch.#process();
292292
}
293+
294+
if (!batches.has(this)) {
295+
this.#commit();
296+
}
293297
}
294298

295299
/**
@@ -433,74 +437,59 @@ export class Batch {
433437
// in other words, we re-run block/async effects with the newly
434438
// committed state, unless the batch in question has a more
435439
// recent value for a given source
436-
if (batches.size > 1) {
437-
this.previous.clear();
438-
439-
var previous_batch = current_batch;
440-
var previous_batch_values = batch_values;
441-
var is_earlier = true;
442-
443-
for (const batch of batches) {
444-
if (batch === this) {
445-
is_earlier = false;
446-
continue;
440+
for (const batch of batches) {
441+
var is_earlier = batch.id < this.id;
442+
443+
/** @type {Source[]} */
444+
var sources = [];
445+
446+
for (const [source, value] of this.current) {
447+
if (batch.current.has(source)) {
448+
if (is_earlier && value !== batch.current.get(source)) {
449+
// bring the value up to date
450+
batch.current.set(source, value);
451+
} else {
452+
// same value or later batch has more recent value,
453+
// no need to re-run these effects
454+
continue;
455+
}
447456
}
448457

449-
/** @type {Source[]} */
450-
const sources = [];
451-
452-
for (const [source, value] of this.current) {
453-
if (batch.current.has(source)) {
454-
if (is_earlier && value !== batch.current.get(source)) {
455-
// bring the value up to date
456-
batch.current.set(source, value);
457-
} else {
458-
// same value or later batch has more recent value,
459-
// no need to re-run these effects
460-
continue;
461-
}
462-
}
458+
sources.push(source);
459+
}
463460

464-
sources.push(source);
465-
}
461+
if (sources.length === 0) {
462+
continue;
463+
}
466464

467-
if (sources.length === 0) {
468-
continue;
469-
}
465+
// Re-run async/block effects that depend on distinct values changed in both batches
466+
var others = [...batch.current.keys()].filter((s) => !this.current.has(s));
467+
if (others.length > 0) {
468+
batch.activate();
470469

471-
// Re-run async/block effects that depend on distinct values changed in both batches
472-
const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
473-
if (others.length > 0) {
474-
batch.activate();
475-
476-
/** @type {Set<Value>} */
477-
const marked = new Set();
478-
/** @type {Map<Reaction, boolean>} */
479-
const checked = new Map();
480-
for (const source of sources) {
481-
mark_effects(source, others, marked, checked);
482-
}
470+
/** @type {Set<Value>} */
471+
var marked = new Set();
483472

484-
if (batch.#roots.length > 0) {
485-
batch.apply();
473+
/** @type {Map<Reaction, boolean>} */
474+
var checked = new Map();
486475

487-
for (const root of batch.#roots) {
488-
batch.#traverse(root, [], []);
489-
}
476+
for (var source of sources) {
477+
mark_effects(source, others, marked, checked);
478+
}
490479

491-
// TODO do we need to do anything with the dummy effect arrays?
480+
if (batch.#roots.length > 0) {
481+
batch.apply();
482+
483+
for (var root of batch.#roots) {
484+
batch.#traverse(root, [], []);
492485
}
493486

494-
batch.deactivate();
487+
// TODO do we need to do anything with the dummy effect arrays?
495488
}
496-
}
497489

498-
current_batch = previous_batch;
499-
batch_values = previous_batch_values;
490+
batch.deactivate();
491+
}
500492
}
501-
502-
this.#skipped_branches.clear();
503-
batches.delete(this);
504493
}
505494

506495
/**
@@ -567,7 +556,10 @@ export class Batch {
567556
}
568557

569558
apply() {
570-
if (!async_mode_flag || (!this.is_fork && batches.size === 1)) return;
559+
if (!async_mode_flag || (!this.is_fork && batches.size === 1)) {
560+
batch_values = null;
561+
return;
562+
}
571563

572564
// if there are multiple batches, we are 'time travelling' —
573565
// we need to override values with the ones in this batch...

0 commit comments

Comments
 (0)