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

Hoisted values passed to slots cause unneeded component updates #3

Open
vatro opened this issue Feb 24, 2022 · 3 comments
Open

Hoisted values passed to slots cause unneeded component updates #3

vatro opened this issue Feb 24, 2022 · 3 comments

Comments

@vatro
Copy link
Owner

vatro commented Feb 24, 2022

Example / REPL: https://svelte.dev/repl/6ca89cd3b45a480db9d70e6f341ab1ad?version=3.46.4
see: /src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts#L195-L203
see: sveltejs/svelte#3124

@vatro
Copy link
Owner Author

vatro commented Feb 24, 2022

Currently we'd need to advise users not to assign/pass/bind top-level declared (hoisted) variables to (slotted) components' props if these are "dynamic" (will change) in order to prevent unneeded/unexpected component updates which would result in more or less significantly lower performance depending on the project itself.

svelte-accmod aims to be more "care less", so actually, this should be changed. In case fiddling with the engine (see code above) breaks too much things, this should be reconsidered.

@vatro
Copy link
Owner Author

vatro commented Feb 24, 2022

First attempts were not successful, was fiddling with the whole InlineComponent logic but couldn't find the right approach:

How to filter out (where exactly? criteria?) dependencies / fragment_dependencies / updates / ??? so that only the (slotted) components having their attributes actually set (using) to dynamic values are being updated via (${name}.$set(${name_changes});) and not their parent components? + is this even the correct fix?

so far touched:

const fragment_dependencies = new Set(this.slots.size ? ['$$scope'] : []);
this.slots.forEach(slot => {
	slot.block.dependencies.forEach(name => {
		// only "child_1" and "child_0"
		// what happens if we add 'hosted_foo' as an attribute / prop to "child_1"? nothing, we're also land here...
		// ... 'hosted_foo' is always inside block.dependencies
		//debugger
		const is_let = slot.scope.is_let(name);
		const variable = renderer.component.var_lookup.get(name);

		
		if (is_let || is_dynamic(variable)) fragment_dependencies.add(name);
		// IMPORTANT  this doesn't help us!
		//if (is_let) fragment_dependencies.add(name);
	});
});
} else {
	dynamic_attributes.forEach((attribute: Attribute) => {
		const dependencies = attribute.get_dependencies();
		// TODO  what about here?
		// "child_1" only!
		//debugger
		if (dependencies.length > 0) {
			const condition = renderer.dirty(dependencies);

			updates.push(b`
				if (${condition}) ${name_changes}.${attribute.name} = ${attribute.get_value(block)};
			`);
		}
	});
}
if (fragment_dependencies.size > 0) {
	// TODO  what about here?
	/*
	debugger

	for (const [key, value] of fragment_dependencies.entries()) {
		console.log(key);
		console.log(value);
		if (key !== '$$scope' && !block.comment?.includes(value)) {
			fragment_dependencies.delete(key);
		}
	  }

	debugger 
	*/
	updates.push(b`
		if (${renderer.dirty(Array.from(fragment_dependencies))}) {
			${name_changes}.$$scope = { dirty: #dirty, ctx: #ctx };
		}`);
}

@vatro
Copy link
Owner Author

vatro commented Dec 11, 2022

TODO: verify this issue again + provide a svelthree example!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant