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

Warn if Foo is reassigned and using <Foo/> #4331

Closed
Conduitry opened this issue Jan 27, 2020 · 4 comments · Fixed by #4409
Closed

Warn if Foo is reassigned and using <Foo/> #4331

Conduitry opened this issue Jan 27, 2020 · 4 comments · Fixed by #4409

Comments

@Conduitry
Copy link
Member

Is your feature request related to a problem? Please describe.
If you make Foo a variable that might change, and then try to use <Foo/>, the component will not re-render when the variable is updated. I don't think it's actually documented anywhere that you need to use <svelte:component> to get the reactivity you want.

Describe the solution you'd like
A compile time warning when you're using a reassigned variable directly as a component tag (or when the variable is passed in via a prop, or maybe some other situations?).

Describe alternatives you've considered
Documenting it instead. Adding some mention of this probably isn't a bad idea, but in general if the compiler can detect a potentially problematic situation, it seems good to have a warning, rather than relying on people to remember this.

How important is this feature to you?
Not especially, personally.

Additional context
Rich had this to say back in November about the decision to keep both <Foo/> and <svelte:component this={Foo}/> when designing Svelte 3:

Dynamic components add a bit of overhead compared to static ones, so IIRC the decision to keep <svelte:component> (other than being able to use expressions) was that in an ambiguous case like export let Component it's probably better to have an explicit way to indicate reactivity. Agree with the 'warn if component constructor is reassigned' idea

@Conduitry
Copy link
Member Author

Something like this

<script>
	import ComponentA from './ComponentA.svelte';
	import ComponentB from './ComponentB.svelte';
	let components = [];
	setTimeout(() => components = [ComponentA, ComponentB], 1000);
</script>

{#each components as Component}
	<Component/>
{/each}

does currently work, which makes this a little more complicated. I'm not sure exactly how to characterize the situations we want to warn in.

@tanhauhau
Copy link
Member

@Conduitry not quite working.

<script>
	import ComponentA from './ComponentA.svelte';
	import ComponentB from './ComponentB.svelte';
	let components = [];
	setTimeout(() => components = [ComponentA, ComponentB], 1000);
	setTimeout(() => components = [ComponentB, ComponentA], 2000);
</script>

{#each components as Component}
	<Component/>
{/each}

the reactivity that works in the previous example is with the components array, still the <Component /> itself is not reactive.

@induratized
Copy link

induratized commented Feb 3, 2020

@Conduitry not quite working.

<script>
	import ComponentA from './ComponentA.svelte';
	import ComponentB from './ComponentB.svelte';
	let components = [];
	setTimeout(() => components = [ComponentA, ComponentB], 1000);
	setTimeout(() => components = [ComponentB, ComponentA], 2000);
</script>

{#each components as Component}
	<Component/>
{/each}

the reactivity that works in the previous example is with the components array, still the <Component /> itself is not reactive.

@tanhauhau the reactivity is achieved when we use the keyed loop (Component) as below

{#each components as Component (Component)} <Component/> {/each}

reactive repl

@Conduitry
Copy link
Member Author

There is now a warning for this in 3.19.0

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

Successfully merging a pull request may close this issue.

3 participants