From 50cc07ec932252bff369223d2a5317079e944e6c Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Fri, 12 Nov 2021 12:47:13 +0700 Subject: [PATCH 01/11] Fix bind:this not working in onMount If a component was instantiated with new Component(), its onMount callback was being called before the bind:this bindings were set up, so that the binding variable was undefined. Now the bind:this bindings are properly bound before the onMount callbacks are called. --- src/runtime/internal/scheduler.ts | 12 +++---- src/runtime/internal/utils.ts | 34 +++++++++++++++++++ .../component-binding-onMount/Mount.svelte | 15 ++++++++ .../component-binding-onMount/_config.js | 11 ++++++ .../component-binding-onMount/main.svelte | 13 +++++++ 5 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 test/runtime/samples/component-binding-onMount/Mount.svelte create mode 100644 test/runtime/samples/component-binding-onMount/_config.js create mode 100644 test/runtime/samples/component-binding-onMount/main.svelte diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 568739e4f862..cc93a6af535b 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -1,7 +1,7 @@ -import { run_all } from './utils'; +import { run_all, Queue } from './utils'; import { set_current_component } from './lifecycle'; -export const dirty_components = []; +export const dirty_components = new Queue(); export const intros = { enabled: false }; export const binding_callbacks = []; @@ -31,17 +31,14 @@ export function add_flush_callback(fn) { flush_callbacks.push(fn); } -let flushing = false; const seen_callbacks = new Set(); export function flush() { - if (flushing) return; - flushing = true; do { // first, call beforeUpdate functions // and update components - for (let i = 0; i < dirty_components.length; i += 1) { - const component = dirty_components[i]; + while (dirty_components.length) { + const component = dirty_components.shift(); set_current_component(component); update(component.$$); } @@ -73,7 +70,6 @@ export function flush() { } update_scheduled = false; - flushing = false; seen_callbacks.clear(); } diff --git a/src/runtime/internal/utils.ts b/src/runtime/internal/utils.ts index 8868e38ee29f..582870ec32a3 100644 --- a/src/runtime/internal/utils.ts +++ b/src/runtime/internal/utils.ts @@ -58,6 +58,40 @@ export function is_empty(obj) { return Object.keys(obj).length === 0; } +export class Queue { + forward: T[]; + reverse: T[]; + + constructor() { + this.forward = []; + this.reverse = []; + } + push(value: T) { + return this.forward.push(value); + } + shift() { + if (this.reverse.length === 0) { + while (this.forward.length) { + this.reverse.push(this.forward.pop()); + } + } + return this.reverse.pop(); + } + get length() { + return this.forward.length + this.reverse.length; + } + set length(len: number) { + if (len === 0) { + this.forward.length = 0; + this.reverse.length = 0; + } else { + while (this.length > len) { + this.shift(); + } + } + } +} + export function validate_store(store, name) { if (store != null && typeof store.subscribe !== 'function') { throw new Error(`'${name}' is not a store with a 'subscribe' method`); diff --git a/test/runtime/samples/component-binding-onMount/Mount.svelte b/test/runtime/samples/component-binding-onMount/Mount.svelte new file mode 100644 index 000000000000..ce3b0c48f806 --- /dev/null +++ b/test/runtime/samples/component-binding-onMount/Mount.svelte @@ -0,0 +1,15 @@ + + +
+

+ Bound? {bound} +

diff --git a/test/runtime/samples/component-binding-onMount/_config.js b/test/runtime/samples/component-binding-onMount/_config.js new file mode 100644 index 000000000000..4ae78b588a1c --- /dev/null +++ b/test/runtime/samples/component-binding-onMount/_config.js @@ -0,0 +1,11 @@ +export default { + async test({ assert, target }) { + assert.htmlEqual(target.innerHTML, ` +
+

+ Bound? true +

+
+ `); + } +}; diff --git a/test/runtime/samples/component-binding-onMount/main.svelte b/test/runtime/samples/component-binding-onMount/main.svelte new file mode 100644 index 000000000000..971b13c1ce68 --- /dev/null +++ b/test/runtime/samples/component-binding-onMount/main.svelte @@ -0,0 +1,13 @@ + + +
From e005bd7f03cbd773a106e7ee05d3c49d7a73bac4 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Wed, 17 Nov 2021 16:26:11 +0700 Subject: [PATCH 02/11] Get unit tests working again --- src/runtime/internal/scheduler.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index cc93a6af535b..1c8cfd9774a9 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -1,5 +1,5 @@ import { run_all, Queue } from './utils'; -import { set_current_component } from './lifecycle'; +import { get_current_component, set_current_component } from './lifecycle'; export const dirty_components = new Queue(); export const intros = { enabled: false }; @@ -34,6 +34,11 @@ export function add_flush_callback(fn) { const seen_callbacks = new Set(); export function flush() { + let current_component = null; + try { + current_component = get_current_component() + } catch {} + do { // first, call beforeUpdate functions // and update components @@ -65,6 +70,8 @@ export function flush() { render_callbacks.length = 0; } while (dirty_components.length); + set_current_component(current_component); + while (flush_callbacks.length) { flush_callbacks.pop()(); } From 96ddf85521cc9ac5b94aabc306051923772ab087 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Wed, 17 Nov 2021 16:31:11 +0700 Subject: [PATCH 03/11] Fix lint errors --- src/runtime/internal/scheduler.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 1c8cfd9774a9..d275920295d1 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -36,8 +36,10 @@ export function flush() { let current_component = null; try { - current_component = get_current_component() - } catch {} + current_component = get_current_component(); + } catch { + // no current component, so leave it as null + } do { // first, call beforeUpdate functions From 83e9cc5d5d405c9cbd20176c0bc636bb782178a6 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Sun, 5 Dec 2021 11:36:42 +0700 Subject: [PATCH 04/11] Remove queue implementation for dirty components Found a way to make it work without changing the dirty components array into a queue structure, reducing bundle size. --- src/runtime/internal/scheduler.ts | 11 ++++++---- src/runtime/internal/utils.ts | 34 ------------------------------- 2 files changed, 7 insertions(+), 38 deletions(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index d275920295d1..73bd5e839571 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -1,7 +1,7 @@ -import { run_all, Queue } from './utils'; +import { run_all } from './utils'; import { get_current_component, set_current_component } from './lifecycle'; -export const dirty_components = new Queue(); +export const dirty_components = []; export const intros = { enabled: false }; export const binding_callbacks = []; @@ -32,6 +32,7 @@ export function add_flush_callback(fn) { } const seen_callbacks = new Set(); +let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { let current_component = null; @@ -44,14 +45,16 @@ export function flush() { do { // first, call beforeUpdate functions // and update components - while (dirty_components.length) { - const component = dirty_components.shift(); + while (flushidx < dirty_components.length) { + const component = dirty_components[flushidx]; + flushidx++; set_current_component(component); update(component.$$); } set_current_component(null); dirty_components.length = 0; + flushidx = 0; while (binding_callbacks.length) binding_callbacks.pop()(); diff --git a/src/runtime/internal/utils.ts b/src/runtime/internal/utils.ts index 582870ec32a3..8868e38ee29f 100644 --- a/src/runtime/internal/utils.ts +++ b/src/runtime/internal/utils.ts @@ -58,40 +58,6 @@ export function is_empty(obj) { return Object.keys(obj).length === 0; } -export class Queue { - forward: T[]; - reverse: T[]; - - constructor() { - this.forward = []; - this.reverse = []; - } - push(value: T) { - return this.forward.push(value); - } - shift() { - if (this.reverse.length === 0) { - while (this.forward.length) { - this.reverse.push(this.forward.pop()); - } - } - return this.reverse.pop(); - } - get length() { - return this.forward.length + this.reverse.length; - } - set length(len: number) { - if (len === 0) { - this.forward.length = 0; - this.reverse.length = 0; - } else { - while (this.length > len) { - this.shift(); - } - } - } -} - export function validate_store(store, name) { if (store != null && typeof store.subscribe !== 'function') { throw new Error(`'${name}' is not a store with a 'subscribe' method`); From 47f90d0a1f30a3e973313bbf8c7544e11ed42993 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Sun, 5 Dec 2021 11:38:27 +0700 Subject: [PATCH 05/11] Don't throw and catch useless errors in flush() Inside flush(), we don't care if there is a current component or not, we just want to save and restore it if there is one. Rather than throwing and catching an error that would happen often, we'll just create another version of get_current_component() that doesn't throw. --- src/runtime/internal/lifecycle.ts | 4 ++++ src/runtime/internal/scheduler.ts | 9 ++------- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/runtime/internal/lifecycle.ts b/src/runtime/internal/lifecycle.ts index bb3df3d29526..028e02d56d7f 100644 --- a/src/runtime/internal/lifecycle.ts +++ b/src/runtime/internal/lifecycle.ts @@ -11,6 +11,10 @@ export function get_current_component() { return current_component; } +export function maybe_get_current_component() { + return current_component; +} + export function beforeUpdate(fn: () => any) { get_current_component().$$.before_update.push(fn); } diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 73bd5e839571..0e5038df8006 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -1,5 +1,5 @@ import { run_all } from './utils'; -import { get_current_component, set_current_component } from './lifecycle'; +import { maybe_get_current_component, set_current_component } from './lifecycle'; export const dirty_components = []; export const intros = { enabled: false }; @@ -35,12 +35,7 @@ const seen_callbacks = new Set(); let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { - let current_component = null; - try { - current_component = get_current_component(); - } catch { - // no current component, so leave it as null - } + let current_component = maybe_get_current_component(); do { // first, call beforeUpdate functions From 86194eb2b958d834e8e063ae9401e9acca0d33eb Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Sun, 5 Dec 2021 11:45:27 +0700 Subject: [PATCH 06/11] Fix lint warning about let vs const --- src/runtime/internal/scheduler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 0e5038df8006..3b01b080d1e1 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -35,7 +35,7 @@ const seen_callbacks = new Set(); let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { - let current_component = maybe_get_current_component(); + const current_component = maybe_get_current_component(); do { // first, call beforeUpdate functions From 407213cd551f271db30b848b724cbe6430e385b1 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Sun, 5 Dec 2021 11:54:42 +0700 Subject: [PATCH 07/11] Don't need extra get_current_component variant --- src/runtime/internal/lifecycle.ts | 4 ---- src/runtime/internal/scheduler.ts | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/runtime/internal/lifecycle.ts b/src/runtime/internal/lifecycle.ts index 028e02d56d7f..bb3df3d29526 100644 --- a/src/runtime/internal/lifecycle.ts +++ b/src/runtime/internal/lifecycle.ts @@ -11,10 +11,6 @@ export function get_current_component() { return current_component; } -export function maybe_get_current_component() { - return current_component; -} - export function beforeUpdate(fn: () => any) { get_current_component().$$.before_update.push(fn); } diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 3b01b080d1e1..8a02629429f1 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -1,5 +1,5 @@ import { run_all } from './utils'; -import { maybe_get_current_component, set_current_component } from './lifecycle'; +import { current_component, set_current_component } from './lifecycle'; export const dirty_components = []; export const intros = { enabled: false }; @@ -35,7 +35,7 @@ const seen_callbacks = new Set(); let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { - const current_component = maybe_get_current_component(); + const saved_component = current_component; do { // first, call beforeUpdate functions @@ -70,7 +70,7 @@ export function flush() { render_callbacks.length = 0; } while (dirty_components.length); - set_current_component(current_component); + set_current_component(saved_component); while (flush_callbacks.length) { flush_callbacks.pop()(); From 195aed145182d1731fd7dbbd0c267ad20f89f2b4 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Sun, 5 Dec 2021 13:33:58 +0700 Subject: [PATCH 08/11] Delete unsightly blank line There shouldn't really be a blank line between the opening brace of a function and its first line of code. Let's get rid of it. --- src/runtime/internal/scheduler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 8a02629429f1..6496500ccd97 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -34,7 +34,6 @@ export function add_flush_callback(fn) { const seen_callbacks = new Set(); let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { - const saved_component = current_component; do { From 91415bf819112018db2a92cf0e0fe00091bc7be4 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Thu, 9 Dec 2021 10:33:12 +0700 Subject: [PATCH 09/11] Restore current component at end of function Since it's not necessary to restore the saved value of current_component before calling the flush_callbacks list, we'll move the restore to the very end of the function, which makes sense as saving it is the first thing the function does. --- src/runtime/internal/scheduler.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 6496500ccd97..93496b84c19c 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -69,14 +69,13 @@ export function flush() { render_callbacks.length = 0; } while (dirty_components.length); - set_current_component(saved_component); - while (flush_callbacks.length) { flush_callbacks.pop()(); } update_scheduled = false; seen_callbacks.clear(); + set_current_component(saved_component); } function update($$) { From fd7586c441f81b6f13c36eac02367db2e05430c3 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Thu, 9 Dec 2021 10:34:27 +0700 Subject: [PATCH 10/11] Add extensive explanatory comments As requested during code review, here's an extensive list of comments explaining the flush() function in more detail for future maintainers. --- src/runtime/internal/scheduler.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 93496b84c19c..50d10143019d 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -31,6 +31,24 @@ export function add_flush_callback(fn) { flush_callbacks.push(fn); } +// flush() calls callbacks in this order: +// 1. All beforeUpdate callbacks, in order: parents before children +// 2. All bind:this callbacks, in reverse order: children before parents. +// 3. All afterUpdate callbacks, in order: parents before children. EXCEPT +// for afterUpdates called during the initial onMount, which are called in +// reverse order: children before parents. +// Since callbacks might update component values, which could trigger another +// call to flush(), the following steps guard against this: +// 1. During beforeUpdate, any updated components will be added to the +// dirty_components array and will cause a reentrant call to flush(). Because +// the flush index is kept outside the function, the reentrant call will pick +// up where the earlier call left off and go through all dirty components. The +// current_component value is saved and restored so that the reentrant call will +// not interfere with the "parent" flush() call. +// 2. bind:this callbacks cannot trigger new flush() calls. +// 3. During afterUpdate, any updated components will NOT have their afterUpdate +// callback called a second time; the seen_callbacks set, outside the flush() +// function, guarantees this behavior. const seen_callbacks = new Set(); let flushidx = 0; // Do *not* move this inside the flush() function export function flush() { From 375ca6204707dd860a75e1872c00aa915251e636 Mon Sep 17 00:00:00 2001 From: Tan Li Hau Date: Sat, 11 Dec 2021 21:59:22 +0800 Subject: [PATCH 11/11] Update src/runtime/internal/scheduler.ts Co-authored-by: Bjorn Lu --- src/runtime/internal/scheduler.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts index 50d10143019d..c0b8e57b08e8 100644 --- a/src/runtime/internal/scheduler.ts +++ b/src/runtime/internal/scheduler.ts @@ -40,11 +40,11 @@ export function add_flush_callback(fn) { // Since callbacks might update component values, which could trigger another // call to flush(), the following steps guard against this: // 1. During beforeUpdate, any updated components will be added to the -// dirty_components array and will cause a reentrant call to flush(). Because -// the flush index is kept outside the function, the reentrant call will pick -// up where the earlier call left off and go through all dirty components. The -// current_component value is saved and restored so that the reentrant call will -// not interfere with the "parent" flush() call. +// dirty_components array and will cause a reentrant call to flush(). Because +// the flush index is kept outside the function, the reentrant call will pick +// up where the earlier call left off and go through all dirty components. The +// current_component value is saved and restored so that the reentrant call will +// not interfere with the "parent" flush() call. // 2. bind:this callbacks cannot trigger new flush() calls. // 3. During afterUpdate, any updated components will NOT have their afterUpdate // callback called a second time; the seen_callbacks set, outside the flush()