Destructuring a store object retuns the whole store if the property does not exist

[fbasso]

Describe the bug
When destructuring a store, the whole store object is returned if the property does not exist:

$: ({shouldBeUndefined} = $store);

It works if used with an existing property, i.e.:

$: ({existingProperty, shouldBeUndefined} = $store);

To Reproduce
You can find the REPL here

Information about your Svelte project:

• Your browser and the version: Firefox 71
• Your operating system: Windows 10
• Svelte version : 3.16.7
• Whether your project uses Webpack or Rollup : Rollup

Severity
Annoying

Thanks!

[Conduitry]

Interesting. The problem is that here where we're defining the $$invalidate function as (i, ret, value = ret) => { ... }, the third argument will default to being the same as the second argument - even if it was explicitly passed undefined, because that's how that works in javascript. I'm not sure what the most elegant way to fix this is. It would be nice to maintain the two-argument version of $$invalidate, because a lot of the time that's all we need. I guess we could look at arguments.length, but that doesn't feel great to me.

[AnatoleLucet]

@Conduitry in which conditions value is passed to $$invalidate? I can't find any time were the compiler renders $$invalidate with a third argument:

svelte/src/compiler/compile/render_dom/Renderer.ts

Lines 151 to 198 in a8b306f

	invalidate(name: string, value?) {
	const variable = this.component.var_lookup.get(name);
	const member = this.context_lookup.get(name);
	if (variable && (variable.subscribable && (variable.reassigned || variable.export_name))) {
	return x`${`$$subscribe_${name}`}($$invalidate(${member.index}, ${value || name}))`;
	}
	if (name[0] === '$' && name[1] !== '$') {
	return x`${name.slice(1)}.set(${value || name})`;
	}
	if (
	variable &&
	!variable.referenced &&
	!variable.is_reactive_dependency &&
	!variable.export_name &&
	!name.startsWith('$$')
	) {
	return value || name;
	}
	if (value) {
	return x`$$invalidate(${member.index}, ${value})`;
	}
	// if this is a reactive declaration, invalidate dependencies recursively
	const deps = new Set([name]);
	deps.forEach(name => {
	const reactive_declarations = this.component.reactive_declarations.filter(x =>
	x.assignees.has(name)
	);
	reactive_declarations.forEach(declaration => {
	declaration.dependencies.forEach(name => {
	deps.add(name);
	});
	});
	});
	// TODO ideally globals etc wouldn't be here in the first place
	const filtered = Array.from(deps).filter(n => this.context_lookup.has(n));
	if (!filtered.length) return null;
	return filtered
	.map(n => x`$$invalidate(${this.context_lookup.get(n).index}, ${n})`)
	.reduce((lhs, rhs) => x`${lhs}, ${rhs}}`);
	}

But anyway, maybe we could check if value is defined or not, do some stuff and then assign him ret's value?

[Conduitry]

The original REPL that reproduces this error is one where $$invalidate() gets called with three arguments - $$invalidate(1, { shouldBeUndefined } = $store, shouldBeUndefined);. Checking whether value is undefined won't help, because (like it is now where it defaults to ret) that doesn't distinguish between it not being passed at all and it being passed as undefined.

[AnatoleLucet]

I see 3 ways to solve this:

1. Using arguments.length as @Conduitry said.

2. Creating two $$invalidate (for example: $$invalidateA, $$invalidateB). One with two arguments (i, ret) and one with three (i, ret, value).

3. Passing a single object though $$invalidate which contain three properties: i, ret, value. Then we can check if value is given in the object with something like:

!!Object.keys({ i: 1, ret: { value: 'test' }, value: undefined }).find(e => e === 'value') // true

!!Object.keys({ i: 1, ret: { value: 'test' } }).find(e => e === 'value') // false

[Conduitry]

I haven't tried this yet, but what seems like it might be a good solution is to make value be the second argument of the $$invalidate() function, and to have ret be the third and default it to value.

[AnatoleLucet]

I guess you mean (i, value, ret = value) => { ... }, then I also guess it should be changed in the compiler?
But even, how could this solve the issue?
I'm sorry if I ask too much questions, I'm just trying to understand the project so I can try to contribute..

[vipero07]

@AnatoleLucet The way the method is currently set up, if value is undefined then it is made to equal ret, which means if value was explicitly undefined, there is no way of knowing (since it is then set to ret). If changed around as @Conduitry proposed, value could be explicitly undefined. I also don't think it would require any rewriting for the compiler, since it would work roughly the same as it did previously, with the exception of being able to handle explicitly undefined values.

[Conduitry]

Fixed in 3.17.0 - https://svelte.dev/repl/52333798bbea4d1b8bb5e249be1cca18?version=3.17.0

[fbasso]

Thanks !

And by the way, congratulation for this great framework :)