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 9 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
58 changes: 36 additions & 22 deletions src/compiler/compile/render_dom/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { b, x, p } from 'code-red';
import Component from '../Component';
import Renderer from './Renderer';
import { CompileOptions } from '../../interfaces';
import { CompileOptions, Var } from '../../interfaces';
import { walk } from 'estree-walker';
import { extract_names } from '../utils/scope';
import { invalidate } from './invalidate';
Expand Down Expand Up @@ -91,7 +91,7 @@ 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; let inject_state: Expression; let capture_state: Expression;

props.forEach(prop => {
const variable = component.var_lookup.get(prop.name);
Expand Down Expand Up @@ -164,27 +164,41 @@ 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 {};
}
`;
// we need to filter out store subscriptions ($x) or $inject_state will try to call .set() on them, leading
// to a crash if store is not writable (and probably not intended behaviour to change store value)
const is_not_sub = (variable: Var) => variable.name.substr(0, 1) !== '$';

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} => {}
`;
const capturable_vars = component.vars.filter(
variable => !variable.module && variable.writable && is_not_sub(variable)
);

if (uses_props || capturable_vars.length > 0) {

const capturable_props = writable_props.filter(is_not_sub);

const local_vars = capturable_vars.filter(variable => !variable.export_name);

const var_names = (variables: Var[]) => variables.map(prop => p`${prop.name}`);

capture_state = x`
({ props: $p = true, local: $l = true } = {}) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Was this left here on purpose? What I had suggested (and what it sounded like you agreed sounded good) was to have separate methods for capturing local state and for capturing props. What I especially don't want is something that returns both at once merged into one object, as that could lead to weird shadowing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry. I had remained focused on the previous discussion of export { ... as ... }. What I had understood was another method to deal with publicly visible (i.e. aliased) names, not to exclude props from $capture_state entirely.

My current implementation only uses internal names, both in what it returns and what it consumes (like the existing one, in fact). Is there really a risk of shadowing in this case? I don't really appreciate if this state could contain other things that could conflict, now or in the future...

If we had a $capture_props method, we'll need a $inject_props too, right? And this pair will have to deal with aliased vs internal names. Also, we need to sort the names of the methods out because what the existing $capture_state currently does is only exposing props as their internal names.

For now, I'm not sure HMR will ever need public names and, if so, I can workaround it with public API by using compiler's output. svelte-devtools also seems content with current $inject_state. So I was hoping efforts for $capture_props or so could be deferred until it was really needed.

Do you think I should split the methods right now? What should be the naming?

Copy link
Member

Choose a reason for hiding this comment

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

Someone could (although it would be crazy) do something like

let foo, bar;
export { foo as bar };

This is why I think props should be entirely excluded from the 'state' methods.

If you think we don't need $capture_props/$inject_props yet for tooling, that's fine, but I don't think $capture_state should have anything to do with props.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, by this I don't mean that $capture_state should remove props from the object it returns, but just that it should disregard any exports and return an object of all of the top-level variables apart from those with injected: true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so, if I understand correctly, you're saying $capture_state should not take any argument and always return every writable variable that is neither injected, nor a store subscription.

That seems to make sense to me so I've updated the PR to do just that, and added a reactive statement in the test case to ensure that they don't end up in the captured state.

...${x`$p && { ${var_names(capturable_props)} }`},
...${x`$l && { ${var_names(local_vars)} }`}
})
`;

inject_state = x`
${$$props} => {
${uses_props && renderer.invalidate('$$props', x`$$props = @assign(@assign({}, $$props), $$new_props)`)}
${capturable_vars.map(prop => b`
if ('${prop.name}' in $$props) ${renderer.invalidate(prop.name, x`${prop.name} = ${$$props}.${prop.name}`)};
`)}
}
`;
} else {
capture_state = x`() => ({})`;
inject_state = x`() => {}`;
}
}

// instrument assignments
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
}
};
158 changes: 158 additions & 0 deletions test/js/samples/capture-inject-state/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/* 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;

const block = {
c: function create() {
p = element("p");
t0 = text(/*$prop*/ ctx[1]);
t1 = space();
t2 = text(/*realName*/ ctx[0]);
t3 = space();
t4 = text(/*local*/ ctx[2]);
t5 = space();
t6 = text(priv);
add_location(p, file, 11, 0, 204);
},
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);
},
p: function update(ctx, [dirty]) {
if (dirty & /*$prop*/ 2) set_data_dev(t0, /*$prop*/ ctx[1]);
if (dirty & /*realName*/ 1) set_data_dev(t2, /*realName*/ ctx[0]);
},
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;
}

const priv = "priv";

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

$$self.$$.on_destroy.push(() => $$unsubscribe_prop());
let { prop } = $$props;
validate_store(prop, "prop");
$$subscribe_prop();
let { alias: realName } = $$props;
let local;
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(3, prop = $$props.prop));
if ("alias" in $$props) $$invalidate(0, realName = $$props.alias);
};

$$self.$capture_state = ({ props: $p = true, local: $l = true } = {}) => ({
...$p && ({ prop, realName }),
...$l && ({ local })
});

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

return [realName, $prop, local, prop];
}

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

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

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

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

if (/*realName*/ ctx[0] === 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;
12 changes: 12 additions & 0 deletions test/js/samples/capture-inject-state/input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<script>
export let prop;

let realName;
export { realName as alias };

let local;

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

$$self.$capture_state = () => {
return { name };
};
$$self.$capture_state = ({ props: $p = true, local: $l = true } = {}) => ({ ...$p && ({ name }), ...$l && ({}) });

$$self.$inject_state = $$props => {
if ("name" in $$props) $$invalidate(0, name = $$props.name);
Expand Down
7 changes: 4 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 @@ -183,9 +183,10 @@ 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 = ({ props: $p = true, local: $l = true } = {}) => ({
...$p && ({ things, foo, bar, baz }),
...$l && ({})
});

$$self.$inject_state = $$props => {
if ("things" in $$props) $$invalidate(0, things = $$props.things);
Expand Down
7 changes: 4 additions & 3 deletions test/js/samples/debug-foo/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,10 @@ function instance($$self, $$props, $$invalidate) {
if ("foo" in $$props) $$invalidate(1, foo = $$props.foo);
};

$$self.$capture_state = () => {
return { things, foo };
};
$$self.$capture_state = ({ props: $p = true, local: $l = true } = {}) => ({
...$p && ({ things, foo }),
...$l && ({})
});

$$self.$inject_state = $$props => {
if ("things" in $$props) $$invalidate(0, things = $$props.things);
Expand Down
7 changes: 4 additions & 3 deletions test/js/samples/debug-hoisted/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ function instance($$self) {
let obj = { x: 5 };
let kobzol = 5;

$$self.$capture_state = () => {
return {};
};
$$self.$capture_state = ({ props: $p = true, local: $l = true } = {}) => ({
...$p && ({}),
...$l && ({ obj, kobzol })
});

$$self.$inject_state = $$props => {
if ("obj" in $$props) $$invalidate(0, obj = $$props.obj);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ function instance($$self, $$props, $$invalidate) {
if ("foo" in $$props) $$invalidate(0, foo = $$props.foo);
};

$$self.$capture_state = () => {
return { foo, bar };
};
$$self.$capture_state = ({ props: $p = true, local: $l = true } = {}) => ({ ...$p && ({ foo }), ...$l && ({ bar }) });

$$self.$inject_state = $$props => {
if ("foo" in $$props) $$invalidate(0, foo = $$props.foo);
Expand Down
8 changes: 3 additions & 5 deletions test/js/samples/loop-protect/expected.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,9 @@ function instance($$self) {
guard_4();
} while (true);

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

$$self.$inject_state = $$props => {
$$self.$inject_state = () => {

};

Expand Down Expand Up @@ -106,4 +104,4 @@ class Component extends SvelteComponentDev {
}
}

export default Component;
export default Component;