Skip to content

fix: object destructuring picks up computed properties #8386

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/compiler/compile/nodes/CatchBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default class CatchBlock extends AbstractBlock {
this.scope = scope.child();
if (parent.catch_node) {
parent.catch_contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
this.scope.add(context.key.name, parent.expression.dependencies, this);
});
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/compile/nodes/ConstTag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export default class ConstTag extends Node {
});
this.expression = new Expression(this.component, this, this.scope, this.node.expression.right);
this.contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
const owner = this.scope.get_owner(context.key.name);
if (owner && owner.type === 'ConstTag' && owner.parent === this.parent) {
this.component.error(this.node, compiler_errors.invalid_const_declaration(context.key.name));
Expand Down
1 change: 1 addition & 0 deletions src/compiler/compile/nodes/EachBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export default class EachBlock extends AbstractBlock {
unpack_destructuring({ contexts: this.contexts, node: info.context, scope, component, context_rest_properties: this.context_rest_properties });

this.contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
this.scope.add(context.key.name, this.expression.dependencies, this);
});

Expand Down
1 change: 1 addition & 0 deletions src/compiler/compile/nodes/ThenBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default class ThenBlock extends AbstractBlock {
this.scope = scope.child();
if (parent.then_node) {
parent.then_contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
this.scope.add(context.key.name, parent.expression.dependencies, this);
});
}
Expand Down
69 changes: 52 additions & 17 deletions src/compiler/compile/nodes/shared/Context.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
import { x } from 'code-red';
import { Node, Identifier, Expression } from 'estree';
import { Node, Identifier, Expression, PrivateIdentifier } from 'estree';
import { walk } from 'estree-walker';
import is_reference, { NodeWithPropertyDefinition } from 'is-reference';
import { clone } from '../../../utils/clone';
import Component from '../../Component';
import flatten_reference from '../../utils/flatten_reference';
import TemplateScope from './TemplateScope';

export interface Context {
export type Context = DestructuredVariable | ComputedProperty;

interface ComputedProperty {
type: 'ComputedProperty';
property_name: string;
key: Expression | PrivateIdentifier;
}

interface DestructuredVariable {
type: 'DestructuredVariable'
key: Identifier;
name?: string;
modifier: (node: Node) => Node;
Expand All @@ -21,26 +30,33 @@ export function unpack_destructuring({
default_modifier = (node) => node,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props = { n: 0 }
}: {
contexts: Context[];
node: Node;
modifier?: Context['modifier'];
default_modifier?: Context['default_modifier'];
modifier?: DestructuredVariable['modifier'];
default_modifier?: DestructuredVariable['default_modifier'];
scope: TemplateScope;
component: Component;
context_rest_properties: Map<string, Node>;
// we want to pass this by reference, as a sort of global variable, because
// if we pass this by value, we could get computed_property_# variable collisions
// when we deal with nested object destructuring
number_of_computed_props?: { n: number };
}) {
if (!node) return;

if (node.type === 'Identifier') {
contexts.push({
type: 'DestructuredVariable',
key: node as Identifier,
modifier,
default_modifier
});
} else if (node.type === 'RestElement') {
contexts.push({
type: 'DestructuredVariable',
key: node.argument as Identifier,
modifier,
default_modifier
Expand All @@ -56,7 +72,8 @@ export function unpack_destructuring({
default_modifier,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
context_rest_properties.set((element.argument as Identifier).name, element);
} else if (element && element.type === 'AssignmentPattern') {
Expand All @@ -76,7 +93,8 @@ export function unpack_destructuring({
)}` as Node,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
} else {
unpack_destructuring({
Expand All @@ -86,7 +104,8 @@ export function unpack_destructuring({
default_modifier,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
}
});
Expand All @@ -105,29 +124,41 @@ export function unpack_destructuring({
default_modifier,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
context_rest_properties.set((property.argument as Identifier).name, property);
} else if (property.type === 'Property') {
const key = property.key;
const value = property.value;

let property_name: any;
let new_modifier: (node: Node) => Node;

if (property.computed) {
// TODO: If the property is computed, ie, { [computed_key]: prop }, the computed_key can be any type of expression.
// e.g { [computedProperty]: ... }
const property_name = `computed_property_${number_of_computed_props.n}`;
number_of_computed_props.n += 1;

contexts.push({
type: 'ComputedProperty',
property_name,
key
});

new_modifier = (node) => x`${modifier(node)}[${property_name}]`;
used_properties.push(x`${property_name}`);
} else if (key.type === 'Identifier') {
// e.g. { someProperty: ... }
property_name = key.name;
const property_name = key.name;
new_modifier = (node) => x`${modifier(node)}.${property_name}`;
used_properties.push(x`"${property_name}"`);
} else if (key.type === 'Literal') {
// e.g. { "property-in-quotes": ... } or { 14: ... }
property_name = key.value;
const property_name = key.value;
new_modifier = (node) => x`${modifier(node)}["${property_name}"]`;
used_properties.push(x`"${property_name}"`);
}

used_properties.push(x`"${property_name}"`);
if (value.type === 'AssignmentPattern') {
// e.g. { property = default } or { property: newName = default }
const n = contexts.length;
Expand All @@ -147,7 +178,8 @@ export function unpack_destructuring({
)}` as Node,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
} else {
// e.g. { property } or { property: newName }
Expand All @@ -158,7 +190,8 @@ export function unpack_destructuring({
default_modifier,
scope,
component,
context_rest_properties
context_rest_properties,
number_of_computed_props
});
}
}
Expand All @@ -174,7 +207,9 @@ function update_reference(
): Node {
const find_from_context = (node: Identifier) => {
for (let i = n; i < contexts.length; i++) {
const { key } = contexts[i];
const cur_context = contexts[i];
if (cur_context.type !== 'DestructuredVariable') continue;
const { key } = cur_context;
if (node.name === key.name) {
throw new Error(`Cannot access '${node.name}' before initialization`);
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/compile/nodes/shared/Expression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ export default class Expression {
// add to get_xxx_context
// child_ctx[x] = function () { ... }
(template_scope.get_owner(deps[0]) as EachBlock).contexts.push({
type: 'DestructuredVariable',
key: func_id,
modifier: () => func_expression,
default_modifier: node => node
Expand Down
12 changes: 11 additions & 1 deletion src/compiler/compile/render_dom/wrappers/AwaitBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import CatchBlock from '../../nodes/CatchBlock';
import { Context } from '../../nodes/shared/Context';
import { Identifier, Literal, Node } from 'estree';
import { add_const_tags, add_const_tags_context } from './shared/add_const_tags';
import Expression from '../../nodes/shared/Expression';

type Status = 'pending' | 'then' | 'catch';

Expand Down Expand Up @@ -69,6 +70,7 @@ class AwaitBlockBranch extends Wrapper {
this.renderer.add_to_context(this.value, true);
} else {
contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
this.renderer.add_to_context(context.key.name, true);
});
this.value = this.block.parent.get_unique_name('value').name;
Expand Down Expand Up @@ -96,7 +98,15 @@ class AwaitBlockBranch extends Wrapper {
}

render_get_context() {
const props = this.is_destructured ? this.value_contexts.map(prop => b`#ctx[${this.block.renderer.context_lookup.get(prop.key.name).index}] = ${prop.default_modifier(prop.modifier(x`#ctx[${this.value_index}]`), name => this.renderer.reference(name))};`) : null;
const props = this.is_destructured ? this.value_contexts.map(prop => {
if (prop.type === 'ComputedProperty') {
const expression = new Expression(this.renderer.component, this.node, this.has_consts(this.node) ? this.node.scope : null, prop.key);
return b`const ${prop.property_name} = ${expression.manipulate(this.block, '#ctx')};`;
} else {
const to_ctx = name => this.renderer.reference(name);
return b`#ctx[${this.block.renderer.context_lookup.get(prop.key.name).index}] = ${prop.default_modifier(prop.modifier(x`#ctx[${this.value_index}]`), to_ctx)};`;
}
}) : null;

const const_tags_props = this.has_consts(this.node) ? add_const_tags(this.block, this.node.const_tags, '#ctx') : null;

Expand Down
13 changes: 12 additions & 1 deletion src/compiler/compile/render_dom/wrappers/EachBlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import ElseBlock from '../../nodes/ElseBlock';
import { Identifier, Node } from 'estree';
import get_object from '../../utils/get_object';
import { add_const_tags, add_const_tags_context } from './shared/add_const_tags';
import Expression from '../../nodes/shared/Expression';

export class ElseBlockWrapper extends Wrapper {
node: ElseBlock;
Expand Down Expand Up @@ -86,6 +87,7 @@ export default class EachBlockWrapper extends Wrapper {
block.add_dependencies(dependencies);

this.node.contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
renderer.add_to_context(context.key.name, true);
});
add_const_tags_context(renderer, this.node.const_tags);
Expand Down Expand Up @@ -147,6 +149,7 @@ export default class EachBlockWrapper extends Wrapper {
const store = object.type === 'Identifier' && object.name[0] === '$' ? object.name.slice(1) : null;

node.contexts.forEach(prop => {
if (prop.type !== 'DestructuredVariable') return;
this.block.bindings.set(prop.key.name, {
object: this.vars.each_block_value,
property: this.index_name,
Expand Down Expand Up @@ -361,7 +364,15 @@ export default class EachBlockWrapper extends Wrapper {
this.else.fragment.render(this.else.block, null, x`#nodes` as Identifier);
}

this.context_props = this.node.contexts.map(prop => b`child_ctx[${renderer.context_lookup.get(prop.key.name).index}] = ${prop.default_modifier(prop.modifier(x`list[i]`), name => renderer.context_lookup.has(name) ? x`child_ctx[${renderer.context_lookup.get(name).index}]` : { type: 'Identifier', name })};`);
this.context_props = this.node.contexts.map(prop => {
if (prop.type === 'DestructuredVariable') {
const to_ctx = (name: string) => renderer.context_lookup.has(name) ? x`child_ctx[${renderer.context_lookup.get(name).index}]` : { type: 'Identifier', name } as Node;
return b`child_ctx[${renderer.context_lookup.get(prop.key.name).index}] = ${prop.default_modifier(prop.modifier(x`list[i]`), to_ctx)};`;
} else {
const expression = new Expression(this.renderer.component, this.node, this.node.scope, prop.key);
return b`const ${prop.property_name} = ${expression.manipulate(block, 'child_ctx')};`;
}
});

if (this.node.has_binding) this.context_props.push(b`child_ctx[${renderer.context_lookup.get(this.vars.each_block_value.name).index}] = list;`);
if (this.node.has_binding || this.node.has_index_binding || this.node.index) this.context_props.push(b`child_ctx[${renderer.context_lookup.get(this.index_name.name).index}] = i;`);
Expand Down
39 changes: 24 additions & 15 deletions src/compiler/compile/render_dom/wrappers/shared/add_const_tags.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,33 @@
import ConstTag from '../../../nodes/ConstTag';
import Block from '../../Block';
import { b, x } from 'code-red';
import { b, Node, x } from 'code-red';
import Renderer from '../../Renderer';
import Expression from '../../../nodes/shared/Expression';

export function add_const_tags(block: Block, const_tags: ConstTag[], ctx: string) {
const const_tags_props = [];
const_tags.forEach((const_tag, i) => {
const name = `#constants_${i}`;
const_tags_props.push(b`const ${name} = ${const_tag.expression.manipulate(block, ctx)}`);
const_tag.contexts.forEach(context => {
const_tags_props.push(b`${ctx}[${block.renderer.context_lookup.get(context.key.name).index}] = ${context.default_modifier(context.modifier({ type: 'Identifier', name }), name => block.renderer.context_lookup.has(name) ? x`${ctx}[${block.renderer.context_lookup.get(name).index}]` : { type: 'Identifier', name })};`);
});
});
return const_tags_props;
const const_tags_props = [];
const_tags.forEach((const_tag, i) => {
const name = `#constants_${i}`;
const_tags_props.push(b`const ${name} = ${const_tag.expression.manipulate(block, ctx)}`);
const to_ctx = (name: string) => block.renderer.context_lookup.has(name) ? x`${ctx}[${block.renderer.context_lookup.get(name).index}]` : { type: 'Identifier', name } as Node;

const_tag.contexts.forEach(context => {
if (context.type === 'DestructuredVariable') {
const_tags_props.push(b`${ctx}[${block.renderer.context_lookup.get(context.key.name).index}] = ${context.default_modifier(context.modifier({ type: 'Identifier', name }), to_ctx)}`);
} else {
const expression = new Expression(block.renderer.component, const_tag, const_tag.scope, context.key);
const_tags_props.push(b`const ${context.property_name} = ${expression.manipulate(block, ctx)}`);
}
});
});
return const_tags_props;
}

export function add_const_tags_context(renderer: Renderer, const_tags: ConstTag[]) {
const_tags.forEach(const_tag => {
const_tag.contexts.forEach(context => {
renderer.add_to_context(context.key.name, true);
});
});
const_tags.forEach(const_tag => {
const_tag.contexts.forEach(context => {
if (context.type !== 'DestructuredVariable') return;
renderer.add_to_context(context.key.name, true);
});
});
}
32 changes: 32 additions & 0 deletions test/runtime/samples/await-then-destruct-computed-props/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export default {
async test({ assert, component, target }) {
await Promise.resolve();

assert.htmlEqual(
target.innerHTML,
`
<p>propA: 3</p>
<p>propB: 7</p>
<p>num: 3</p>
<p>rest: {"prop3":{"prop9":9,"prop10":10}}</p>
<p>propZ: 5</p>
<p>propY: 6</p>
<p>rest: {"propX":7,"propW":8}</p>
`
);

await (component.object = Promise.resolve({ prop1: 'one', prop2: 'two', prop3: { prop7: 'seven' }, prop4: { prop10: 'ten' }}));
assert.htmlEqual(
target.innerHTML,
`
<p>propA: seven</p>
<p>propB: ten</p>
<p>num: 5</p>
<p>rest: {"prop1":"one","prop2":"two"}</p>
<p>propZ: 5</p>
<p>propY: 6</p>
<p>rest: {"propX":7,"propW":8}</p>
`
);
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<script>
export let object = Promise.resolve({ prop1: { prop4: 2, prop5: 3 }, prop2: { prop6: 5, prop7: 6, prop8: 7 }, prop3: { prop9: 9, prop10: 10 } });
const objectReject = Promise.reject({ propZ: 5, propY: 6, propX: 7, propW: 8 });

let num = 1;
const prop = 'prop';
</script>

{#await object then { [`prop${num++}`]: { [`prop${num + 3}`]: propA }, [`prop${num++}`]: { [`prop${num + 5}`]: propB }, ...rest }}
<p>propA: {propA}</p>
<p>propB: {propB}</p>
<p>num: {num}</p>
<p>rest: {JSON.stringify(rest)}</p>
{/await}

{#await objectReject then value}
resolved
{:catch { [`${prop}Z`]: propZ, [`${prop}Y`]: propY, ...rest }}
<p>propZ: {propZ}</p>
<p>propY: {propY}</p>
<p>rest: {JSON.stringify(rest)}</p>
{/await}

Loading