Skip to content

fix: #4265 prevent double update when bind to object #8992

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

fix: #4265 prevent double update when bind to object #8992

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bfeda78
fix: #4265 prevent double update when bind to object
Jul 18, 2023
1cc1280
fix: correctly detect bound value mutation at init phase
Jul 19, 2023
0341e0b
chore: add testcases
Jul 19, 2023
841b641
fix: invalidate props in `main_execution_context`
Jul 23, 2023
0e5e471
fix: update testcase
Jul 23, 2023
34dcc5d
try unskipping the other test
dummdidumm Sep 20, 2023
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
14 changes: 10 additions & 4 deletions packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ export default class InlineComponentWrapper extends Wrapper {
block.maintain_context = true; // TODO put this somewhere more logical
}
block.chunks.init.push(b`
function ${id}(#value) {
${callee}(${args});
function ${id}(#value, #skip_binding) {
${callee}(#skip_binding, ${args});
}
`);
let invalidate_binding = b`
Expand All @@ -360,9 +360,15 @@ export default class InlineComponentWrapper extends Wrapper {
}
`;
}
// `skip_binding` is set by runtime/internal `bind()` function only at first call
// this prevents child -> parent backflow that triggers unnecessary $$.update (#4265)
// the flag is used to detech reactive mutation to object in `child.$$.update`
// ignore this flag when parent_value !== child_value
const body = b`
function ${id}(${params}) {
${invalidate_binding}
function ${id}(#skip_binding, ${params}) {
if (!#skip_binding || ${lhs} !== #value) {
${invalidate_binding}
}
Comment on lines +369 to +371
Copy link
Contributor Author

@hackape hackape Jul 19, 2023

Choose a reason for hiding this comment

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

if (!#skip_binding || ${lhs} !== #value) guard prevents the unnecessary backflow of case 2, also ${lhs} !== #value check allows backflow for case 1 and 4.1.

Case 2: Child does not touch passed-in prop value, regardless it's primitive or object, backflow is unnecessary
Case 1 and 4.1: Child value is referentially different from Parent value

}
`;
component.partly_hoisted.push(body);
Expand Down
14 changes: 9 additions & 5 deletions packages/svelte/src/runtime/internal/Component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ import { transition_in } from './transitions.js';

/** @returns {void} */
export function bind(component, name, callback) {
const index = component.$$.props[name];
if (index !== undefined) {
component.$$.bound[index] = callback;
callback(component.$$.ctx[index]);
const i = component.$$.props[name];
if (i !== undefined) {
let dirty = false;
if (component.$$.bound[i]) dirty = true;
component.$$.bound[i] = callback;
// first binding call, if child value is not yet dirty, skip to prevent unnecessary backflow
callback(component.$$.ctx[i], /** skip_binding */ !dirty);
Copy link
Contributor Author

@hackape hackape Jul 19, 2023

Choose a reason for hiding this comment

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

The bind runtime function triggers the first backflow during flush, here I pass a skip_binding flag to child_value_binding. The flag is determined by $$.bound[i] dirty mark. This takes care of case 2 and 3.

Case 2: Child does not touch passed-in prop value
Case 3: Child modifies the value inside $: reactive statements

}
}

Expand Down Expand Up @@ -125,8 +128,9 @@ export function init(
? instance(component, options.props || {}, (i, ret, ...rest) => {
const value = rest.length ? rest[0] : ret;
if ($$.ctx && not_equal($$.ctx[i], ($$.ctx[i] = value))) {
if (!$$.skip_bound && $$.bound[i]) $$.bound[i](value);
if (!$$.skip_bound && is_function($$.bound[i])) $$.bound[i](value);
if (ready) make_dirty(component, i);
else $$.bound[i] = true; // dirty flag consumed in bind()
Copy link
Contributor Author

@hackape hackape Jul 19, 2023

Choose a reason for hiding this comment

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

The $$invalidate runtime function, when ready == false sets $$.bound[i] = true in order to cater for case 3. This mark is essentially the same as $$.dirty which is not available before ready.

}
return ret;
})
Expand Down