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

Make $capture_state capture local state #3822

Merged
merged 24 commits into from
Feb 23, 2020
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cf3fe22
capture local variables in $capture_state
rixo Oct 29, 2019
0239ff3
Merge remote-tracking branch 'upstream/master' into hmr-capture-state
rixo Nov 4, 2019
b32a022
Merge branch 'master' of github.com:sveltejs/svelte into hmr-capture-…
Rich-Harris Nov 14, 2019
a99cd8a
update tests
Rich-Harris Nov 14, 2019
16398f9
Merge remote-tracking branch 'upstream/master' into hmr-capture-state
rixo Dec 3, 2019
1f84174
shorter & less ambiguous param names
rixo Dec 3, 2019
d1f490b
fix TS warnings
rixo Dec 3, 2019
1521c67
Merge branch 'hmr-capture-state' of github.com:rixo/svelte into hmr-c…
rixo Dec 3, 2019
8cdcdc0
add test of $capture_state & $inject_state behaviour
rixo Dec 4, 2019
00ef7bd
remove specific handling of props from $capture_state
rixo Dec 4, 2019
002bed8
fix TS error
rixo Dec 4, 2019
eb4dcfc
capture store subscriptions in $capture_state
rixo Dec 4, 2019
8989476
fix lint error
rixo Dec 4, 2019
3b5a7df
add $$inject special prop, clean $capture_state & $inject_state
rixo Dec 5, 2019
67bafea
Merge branch 'master' into pr/3822
Conduitry Dec 9, 2019
ae5d759
Merge master into pr/3822
rixo Dec 20, 2019
1f6d759
Merge remote-tracking branch 'upstream/master' into hmr-capture-state
rixo Dec 23, 2019
bed3964
Merge remote-tracking branch 'upstream/master' into hmr-capture-state
rixo Dec 30, 2019
e478e1f
Merge remote-tracking branch 'upstream/master' into hmr-capture-state
rixo Dec 30, 2019
7ba1d76
Merge branch 'master' into pr/3822
Conduitry Feb 18, 2020
63a56ad
fix test
Conduitry Feb 18, 2020
7e5578f
tidy
Conduitry Feb 18, 2020
dde2b21
tidy
Conduitry Feb 18, 2020
1a38d04
simplify handling of noop $capture_state/$inject_state
Conduitry Feb 19, 2020
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
63 changes: 40 additions & 23 deletions src/compiler/compile/render_dom/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ export default function dom(
const accessors = [];

const not_equal = component.component_options.immutable ? x`@not_equal` : x`@safe_not_equal`;
let dev_props_check; let inject_state; let capture_state;
let dev_props_check: Node[] | Node;
let inject_state: Expression;
let capture_state: Expression;
let props_inject: Node[] | Node;

props.forEach(prop => {
const variable = component.var_lookup.get(prop.name);
Expand Down Expand Up @@ -164,27 +167,32 @@ export default function dom(
`;
}

capture_state = (uses_props || writable_props.length > 0) ? x`
() => {
return { ${component.vars.filter(prop => prop.writable).map(prop => p`${prop.name}`)} };
}
` : x`
() => {
return {};
}
`;
const capturable_vars = component.vars.filter(v => !v.internal && !v.name.startsWith('$$'));

const writable_vars = component.vars.filter(variable => !variable.module && variable.writable);
inject_state = (uses_props || writable_vars.length > 0) ? x`
${$$props} => {
${uses_props && renderer.invalidate('$$props', x`$$props = @assign(@assign({}, $$props), $$new_props)`)}
${writable_vars.map(prop => b`
if ('${prop.name}' in $$props) ${renderer.invalidate(prop.name, x`${prop.name} = ${$$props}.${prop.name}`)};
`)}
}
` : x`
${$$props} => {}
`;
capture_state = capturable_vars.length > 0
? x`() => ({ ${capturable_vars.map(prop => p`${prop.name}`)} })`
: x`@noop`;

const injectable_vars = capturable_vars.filter(v => !v.module && v.writable && v.name[0] !== '$');

if (uses_props || injectable_vars.length > 0) {
inject_state = x`
${$$props} => {
${uses_props && renderer.invalidate('$$props', x`$$props = @assign(@assign({}, $$props), $$new_props)`)}
${injectable_vars.map(
v => b`if ('${v.name}' in $$props) ${renderer.invalidate(v.name, x`${v.name} = ${$$props}.${v.name}`)};`
)}
}
`;

props_inject = b`
if ($$props && "$$inject" in $$props) {
$$self.$inject_state($$props.$$inject);
}
`;
} else {
inject_state = x`@noop`;
}
}

// instrument assignments
Expand Down Expand Up @@ -246,7 +254,12 @@ export default function dom(
}

const args = [x`$$self`];
if (props.length > 0 || component.has_reactive_assignments || component.slots.size > 0) {
const has_invalidate = props.length > 0 ||
component.has_reactive_assignments ||
component.slots.size > 0 ||
capture_state ||
Copy link
Member

Choose a reason for hiding this comment

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

Is the intent here (and then again below) to check whether there's a non-noop capture_state/inject_state? Right now, won't this will be truthy whenever we're compiling in dev mode?

Copy link
Member

Choose a reason for hiding this comment

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

Hm when I made a change to this effect locally, what ended up happening was that in the debug-no-dependencies test, no instance function was generated at all, and so $$self.$capture_state/$$self.$inject_state weren't set, even to noop. So perhaps what's here now is correct.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative would be to make $capture_state and $inject_state actually exist as instance methods on SvelteComponentDev (as noops), and have them be overridden in instance when necessary - which I might actually like better. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, thanks :)

So yeah, it was always true in dev mode (before your change) but I just wanted to write out the real reason, that it was needed for $capture_state and $inject_state.

I agree that having them on SvelteComponentDev is far better. It also explicits these methods' contract by making it clear that they are always present for all components in dev mode. It's great.

inject_state;
if (has_invalidate) {
args.push(x`$$props`, x`$$invalidate`);
}

Expand Down Expand Up @@ -294,7 +307,9 @@ export default function dom(
uses_props ||
component.partly_hoisted.length > 0 ||
initial_context.length > 0 ||
component.reactive_declarations.length > 0
component.reactive_declarations.length > 0 ||
capture_state ||
inject_state
);

const definition = has_definition
Expand Down Expand Up @@ -409,6 +424,8 @@ export default function dom(

${injected.map(name => b`let ${name};`)}

${/* before reactive declarations */ props_inject}

${reactive_declarations.length > 0 && b`
$$self.$$.update = () => {
${reactive_declarations}
Expand Down
5 changes: 5 additions & 0 deletions test/js/samples/capture-inject-state/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default {
options: {
dev: true
}
};
197 changes: 197 additions & 0 deletions test/js/samples/capture-inject-state/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
/* generated by Svelte vX.Y.Z */
import {
SvelteComponentDev,
add_location,
append_dev,
detach_dev,
dispatch_dev,
element,
init,
insert_dev,
noop,
safe_not_equal,
set_data_dev,
space,
subscribe,
text,
validate_store
} from "svelte/internal";

const file = undefined;

function create_fragment(ctx) {
let p;
let t0;
let t1;
let t2;
let t3;
let t4;
let t5;
let t6;
let t7;
let t8;
let t9;
let t10;

const block = {
c: function create() {
p = element("p");
t0 = text(/*prop*/ ctx[0]);
t1 = space();
t2 = text(/*realName*/ ctx[1]);
t3 = space();
t4 = text(/*local*/ ctx[3]);
t5 = space();
t6 = text(priv);
t7 = space();
t8 = text(/*$prop*/ ctx[2]);
t9 = space();
t10 = text(/*shadowedByModule*/ ctx[4]);
add_location(p, file, 22, 0, 430);
},
l: function claim(nodes) {
throw new Error("options.hydrate only works if the component was compiled with the `hydratable: true` option");
},
m: function mount(target, anchor) {
insert_dev(target, p, anchor);
append_dev(p, t0);
append_dev(p, t1);
append_dev(p, t2);
append_dev(p, t3);
append_dev(p, t4);
append_dev(p, t5);
append_dev(p, t6);
append_dev(p, t7);
append_dev(p, t8);
append_dev(p, t9);
append_dev(p, t10);
},
p: function update(ctx, [dirty]) {
if (dirty & /*prop*/ 1) set_data_dev(t0, /*prop*/ ctx[0]);
if (dirty & /*realName*/ 2) set_data_dev(t2, /*realName*/ ctx[1]);
if (dirty & /*$prop*/ 4) set_data_dev(t8, /*$prop*/ ctx[2]);
},
i: noop,
o: noop,
d: function destroy(detaching) {
if (detaching) detach_dev(p);
}
};

dispatch_dev("SvelteRegisterBlock", {
block,
id: create_fragment.name,
type: "component",
source: "",
ctx
});

return block;
}

let moduleLiveBinding;
const moduleContantProps = 4;
let moduleLet;
const moduleConst = 2;
let shadowedByModule;
const priv = "priv";

function instance($$self, $$props, $$invalidate) {
let $prop,
$$unsubscribe_prop = noop,
$$subscribe_prop = () => ($$unsubscribe_prop(), $$unsubscribe_prop = subscribe(prop, $$value => $$invalidate(2, $prop = $$value)), prop);

$$self.$$.on_destroy.push(() => $$unsubscribe_prop());
let { prop } = $$props;
validate_store(prop, "prop");
$$subscribe_prop();
let { alias: realName } = $$props;
let local;
let shadowedByModule;
const writable_props = ["prop", "alias"];

Object.keys($$props).forEach(key => {
if (!~writable_props.indexOf(key) && key.slice(0, 2) !== "$$") console.warn(`<Component> was created with unknown prop '${key}'`);
});

$$self.$set = $$props => {
if ("prop" in $$props) $$subscribe_prop($$invalidate(0, prop = $$props.prop));
if ("alias" in $$props) $$invalidate(1, realName = $$props.alias);
};

$$self.$capture_state = () => ({
moduleLiveBinding,
moduleContantProps,
moduleLet,
moduleConst,
shadowedByModule,
prop,
realName,
local,
priv,
shadowedByModule,
computed,
$prop
});

$$self.$inject_state = $$props => {
if ("prop" in $$props) $$subscribe_prop($$invalidate(0, prop = $$props.prop));
if ("realName" in $$props) $$invalidate(1, realName = $$props.realName);
if ("local" in $$props) $$invalidate(3, local = $$props.local);
if ("shadowedByModule" in $$props) $$invalidate(4, shadowedByModule = $$props.shadowedByModule);
if ("computed" in $$props) computed = $$props.computed;
};

let computed;

if ($$props && "$$inject" in $$props) {
$$self.$inject_state($$props.$$inject);
}

$: computed = local * 2;
return [prop, realName, $prop, local, shadowedByModule];
}

class Component extends SvelteComponentDev {
constructor(options) {
super(options);
init(this, options, instance, create_fragment, safe_not_equal, { prop: 0, alias: 1 });

dispatch_dev("SvelteRegisterComponent", {
component: this,
tagName: "Component",
options,
id: create_fragment.name
});

const { ctx } = this.$$;
const props = options.props || {};

if (/*prop*/ ctx[0] === undefined && !("prop" in props)) {
console.warn("<Component> was created without expected prop 'prop'");
}

if (/*realName*/ ctx[1] === undefined && !("alias" in props)) {
console.warn("<Component> was created without expected prop 'alias'");
}
}

get prop() {
throw new Error("<Component>: Props cannot be read directly from the component instance unless compiling with 'accessors: true' or '<svelte:options accessors/>'");
}

set prop(value) {
throw new Error("<Component>: Props cannot be set directly on the component instance unless compiling with 'accessors: true' or '<svelte:options accessors/>'");
}

get alias() {
throw new Error("<Component>: Props cannot be read directly from the component instance unless compiling with 'accessors: true' or '<svelte:options accessors/>'");
}

set alias(value) {
throw new Error("<Component>: Props cannot be set directly on the component instance unless compiling with 'accessors: true' or '<svelte:options accessors/>'");
}
}

export default Component;
export { moduleLiveBinding, moduleContantProps };
23 changes: 23 additions & 0 deletions test/js/samples/capture-inject-state/input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<script context="module">
export let moduleLiveBinding;
export const moduleContantProps = 4;
let moduleLet;
const moduleConst = 2;
let shadowedByModule;
</script>
<script>
export let prop;

let realName;
export { realName as alias };

let local;

const priv = 'priv';

$: computed = local * 2;

let shadowedByModule;
</script>
<!-- NOTE $prop ensures store subscriptions are not part of captured state -->
<p>{prop} {realName} {local} {priv} {$prop} {shadowedByModule}</p>
8 changes: 5 additions & 3 deletions test/js/samples/debug-empty/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,16 @@ function instance($$self, $$props, $$invalidate) {
if ("name" in $$props) $$invalidate(0, name = $$props.name);
};

$$self.$capture_state = () => {
return { name };
};
$$self.$capture_state = () => ({ name });

$$self.$inject_state = $$props => {
if ("name" in $$props) $$invalidate(0, name = $$props.name);
};

if ($$props && "$$inject" in $$props) {
$$self.$inject_state($$props.$$inject);
}

return [name];
}

Expand Down
8 changes: 5 additions & 3 deletions test/js/samples/debug-foo-bar-baz-things/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,7 @@ function instance($$self, $$props, $$invalidate) {
if ("baz" in $$props) $$invalidate(3, baz = $$props.baz);
};

$$self.$capture_state = () => {
return { things, foo, bar, baz };
};
$$self.$capture_state = () => ({ things, foo, bar, baz });

$$self.$inject_state = $$props => {
if ("things" in $$props) $$invalidate(0, things = $$props.things);
Expand All @@ -197,6 +195,10 @@ function instance($$self, $$props, $$invalidate) {
if ("baz" in $$props) $$invalidate(3, baz = $$props.baz);
};

if ($$props && "$$inject" in $$props) {
$$self.$inject_state($$props.$$inject);
}

return [things, foo, bar, baz];
}

Expand Down
8 changes: 5 additions & 3 deletions test/js/samples/debug-foo/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,17 @@ function instance($$self, $$props, $$invalidate) {
if ("foo" in $$props) $$invalidate(1, foo = $$props.foo);
};

$$self.$capture_state = () => {
return { things, foo };
};
$$self.$capture_state = () => ({ things, foo });

$$self.$inject_state = $$props => {
if ("things" in $$props) $$invalidate(0, things = $$props.things);
if ("foo" in $$props) $$invalidate(1, foo = $$props.foo);
};

if ($$props && "$$inject" in $$props) {
$$self.$inject_state($$props.$$inject);
}

return [things, foo];
}

Expand Down
Loading