-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 1 commit
5c1235a
4fd93a1
fd20fd5
434434f
749fa0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
/* generated by Svelte vX.Y.Z */ | ||
import { | ||
SvelteComponent, | ||
detach, | ||
element, | ||
init, | ||
insert, | ||
noop, | ||
safe_not_equal, | ||
space, | ||
toggle_class | ||
} from "svelte/internal"; | ||
|
||
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; | ||
|
||
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"); | ||
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, "static1", nonReactiveModuleVar); | ||
toggle_class(div4, "static1", nonReactiveGlobal); | ||
toggle_class(div5, "static1", nonReactiveModuleVar && nonReactiveGlobal); | ||
}, | ||
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); | ||
}, | ||
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); | ||
} | ||
}, | ||
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); | ||
} | ||
}; | ||
} | ||
|
||
let nonReactiveModuleVar = Math.random(); | ||
let reactiveModuleVar = Math.random(); | ||
|
||
function instance($$self, $$props, $$invalidate) { | ||
nonReactiveGlobal = Math.random(); | ||
const reactiveConst = { x: Math.random() }; | ||
reactiveModuleVar += 1; | ||
|
||
if (Math.random()) { | ||
reactiveConst.x += 1; | ||
} | ||
|
||
return [reactiveConst]; | ||
} | ||
|
||
class Component extends SvelteComponent { | ||
constructor(options) { | ||
super(); | ||
init(this, options, instance, create_fragment, safe_not_equal, {}); | ||
} | ||
} | ||
|
||
export default Component; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<script context="module"> | ||
let nonReactiveModuleVar = Math.random(); | ||
let reactiveModuleVar = Math.random(); | ||
</script> | ||
|
||
<script> | ||
nonReactiveGlobal = Math.random(); | ||
dummdidumm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const reactiveConst = {x: Math.random()}; | ||
|
||
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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
||
<!--These shouldn't get updates because they're purely non-reactive--> | ||
<div class:static1={nonReactiveModuleVar}></div> | ||
<div class:static1={nonReactiveGlobal}></div> | ||
<div class:static1={nonReactiveModuleVar && nonReactiveGlobal}></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename variable
v
to something meaningful, such asvariable
.Beyond that, this PR is great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, completely missed that! Will push a commit to fix that