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

Don't create class update functions when dependencies aren't reactive #5926

Merged
merged 5 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from 4 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
11 changes: 10 additions & 1 deletion src/compiler/compile/render_dom/wrappers/Element/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import Action from '../../../nodes/Action';
import MustacheTagWrapper from '../MustacheTag';
import RawMustacheTagWrapper from '../RawMustacheTag';
import create_slot_block from './create_slot_block';
import is_dynamic from '../shared/is_dynamic';

interface BindingGroup {
events: string[];
Expand Down Expand Up @@ -898,10 +899,18 @@ export default class ElementWrapper extends Wrapper {
const all_dependencies = this.class_dependencies.concat(...dependencies);
const condition = block.renderer.dirty(all_dependencies);

block.chunks.update.push(b`
// If all of the dependencies are non-dynamic (don't get updated) then there is no reason
// to add an updater for this.
const any_dynamic_dependencies = all_dependencies.some((dep) => {
const variable = this.renderer.component.var_lookup.get(dep);
return !variable || is_dynamic(variable);
});
if (any_dynamic_dependencies) {
block.chunks.update.push(b`
if (${condition}) {
${updater}
}`);
}
}
});
}
Expand Down
167 changes: 167 additions & 0 deletions test/js/samples/reactive-class-optimized/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/* generated by Svelte vX.Y.Z */
import {
SvelteComponent,
component_subscribe,
detach,
element,
init,
insert,
noop,
safe_not_equal,
space,
subscribe,
toggle_class
} from "svelte/internal";

import { reactiveStoreVal, unreactiveExport } from "./store";

function create_fragment(ctx) {
let div0;
let t0;
let div1;
let t1;
let div2;
let t2;
let div3;
let t3;
let div4;
let t4;
let div5;
let t5;
let div6;
let t6;
let div7;
let t7;
let div8;

return {
c() {
div0 = element("div");
t0 = space();
div1 = element("div");
t1 = space();
div2 = element("div");
t2 = space();
div3 = element("div");
t3 = space();
div4 = element("div");
t4 = space();
div5 = element("div");
t5 = space();
div6 = element("div");
t6 = space();
div7 = element("div");
t7 = space();
div8 = element("div");
toggle_class(div0, "update1", reactiveModuleVar);
toggle_class(div1, "update2", /*reactiveConst*/ ctx[0].x);
toggle_class(div2, "update3", nonReactiveGlobal && /*reactiveConst*/ ctx[0].x);
toggle_class(div3, "update4", /*$reactiveStoreVal*/ ctx[2]);
toggle_class(div4, "update5", /*$reactiveDeclaration*/ ctx[3]);
toggle_class(div5, "static1", nonReactiveModuleVar);
toggle_class(div6, "static2", nonReactiveGlobal);
toggle_class(div7, "static3", nonReactiveModuleVar && nonReactiveGlobal);
toggle_class(div8, "static4", unreactiveExport);
},
m(target, anchor) {
insert(target, div0, anchor);
insert(target, t0, anchor);
insert(target, div1, anchor);
insert(target, t1, anchor);
insert(target, div2, anchor);
insert(target, t2, anchor);
insert(target, div3, anchor);
insert(target, t3, anchor);
insert(target, div4, anchor);
insert(target, t4, anchor);
insert(target, div5, anchor);
insert(target, t5, anchor);
insert(target, div6, anchor);
insert(target, t6, anchor);
insert(target, div7, anchor);
insert(target, t7, anchor);
insert(target, div8, anchor);
},
p(ctx, [dirty]) {
if (dirty & /*reactiveModuleVar*/ 0) {
toggle_class(div0, "update1", reactiveModuleVar);
}

if (dirty & /*reactiveConst*/ 1) {
toggle_class(div1, "update2", /*reactiveConst*/ ctx[0].x);
}

if (dirty & /*nonReactiveGlobal, reactiveConst*/ 1) {
toggle_class(div2, "update3", nonReactiveGlobal && /*reactiveConst*/ ctx[0].x);
}

if (dirty & /*$reactiveStoreVal*/ 4) {
toggle_class(div3, "update4", /*$reactiveStoreVal*/ ctx[2]);
}

if (dirty & /*$reactiveDeclaration*/ 8) {
toggle_class(div4, "update5", /*$reactiveDeclaration*/ ctx[3]);
}
},
i: noop,
o: noop,
d(detaching) {
if (detaching) detach(div0);
if (detaching) detach(t0);
if (detaching) detach(div1);
if (detaching) detach(t1);
if (detaching) detach(div2);
if (detaching) detach(t2);
if (detaching) detach(div3);
if (detaching) detach(t3);
if (detaching) detach(div4);
if (detaching) detach(t4);
if (detaching) detach(div5);
if (detaching) detach(t5);
if (detaching) detach(div6);
if (detaching) detach(t6);
if (detaching) detach(div7);
if (detaching) detach(t7);
if (detaching) detach(div8);
}
};
}

let nonReactiveModuleVar = Math.random();
let reactiveModuleVar = Math.random();

function instance($$self, $$props, $$invalidate) {
let reactiveDeclaration;
let $reactiveStoreVal;

let $reactiveDeclaration,
$$unsubscribe_reactiveDeclaration = noop,
$$subscribe_reactiveDeclaration = () => ($$unsubscribe_reactiveDeclaration(), $$unsubscribe_reactiveDeclaration = subscribe(reactiveDeclaration, $$value => $$invalidate(3, $reactiveDeclaration = $$value)), reactiveDeclaration);

component_subscribe($$self, reactiveStoreVal, $$value => $$invalidate(2, $reactiveStoreVal = $$value));
$$self.$$.on_destroy.push(() => $$unsubscribe_reactiveDeclaration());
nonReactiveGlobal = Math.random();
const reactiveConst = { x: Math.random() };
reactiveModuleVar += 1;

if (Math.random()) {
reactiveConst.x += 1;
}

$$self.$$.update = () => {
if ($$self.$$.dirty & /*reactiveModuleVar*/ 0) {
$: $$subscribe_reactiveDeclaration($$invalidate(1, reactiveDeclaration = reactiveModuleVar * 2));
}
};

return [reactiveConst, reactiveDeclaration, $reactiveStoreVal, $reactiveDeclaration];
}

class Component extends SvelteComponent {
constructor(options) {
super();
init(this, options, instance, create_fragment, safe_not_equal, {});
}
}

export default Component;
31 changes: 31 additions & 0 deletions test/js/samples/reactive-class-optimized/input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<script context="module">
let nonReactiveModuleVar = Math.random();
let reactiveModuleVar = Math.random();
</script>

<script>
import { reactiveStoreVal, unreactiveExport } from './store';

nonReactiveGlobal = Math.random();
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
const reactiveConst = {x: Math.random()};

$: reactiveDeclaration = reactiveModuleVar * 2;

reactiveModuleVar += 1;
if (Math.random()) {
reactiveConst.x += 1;
}
</script>

<!--These should all get updaters because they have at least one reactive dependency-->
<div class:update1={reactiveModuleVar}></div>
Copy link
Member

Choose a reason for hiding this comment

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

actually moduleVar will never be reactive as it does not belong to any component instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! If it's okay with you, I would like to file an issue for that and solve it in a different PR. It's being indicated as being reactive by is_dynamic and I worry that changing that function to fix this will have other effects and would prefer to isolate that (and the testing for it) to a new PR.

My PR won't cause a regression even though it treats the module var as reactive. https://svelte.dev/repl/3a6d024461d74e43b5fdf7ea144a9a80?version=3.32.0 shows that Svelte already creates a toggle_class for a module var in the update function so my PR won't change that behavior at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed an issue at #5943 and I'll hopefully have a fix out within a week

<div class:update2={reactiveConst.x}></div>
<div class:update3={nonReactiveGlobal && reactiveConst.x}></div>
<div class:update4={$reactiveStoreVal}></div>
<div class:update5={$reactiveDeclaration}></div>

<!--These shouldn't get updates because they're purely non-reactive-->
<div class:static1={nonReactiveModuleVar}></div>
<div class:static2={nonReactiveGlobal}></div>
<div class:static3={nonReactiveModuleVar && nonReactiveGlobal}></div>
<div class:static4={unreactiveExport}></div>
4 changes: 4 additions & 0 deletions test/js/samples/reactive-class-optimized/store.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { writable } from '../../../../store';

export const reactiveStoreVal = writable(0);
export const unreactiveExport = true;