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

Track which variables need to be reactive #1952

Closed
Rich-Harris opened this issue Jan 3, 2019 · 3 comments
Closed

Track which variables need to be reactive #1952

Rich-Harris opened this issue Jan 3, 2019 · 3 comments
Milestone

Comments

@Rich-Harris
Copy link
Member

REPL. We're not taking full advantage of the information available to us. This code...

<script>
  let name = 'world';
</script>

<h1>Hello {name}!</h1>

...should result in this output...

function create_fragment($$, ctx) {
  var h1, text0, text1, text2, current;

  return {
    c() {
      h1 = createElement("h1");
      text0 = createText("Hello ");
      text1 = createText(ctx.name);
      text2 = createText("!");
    },

    m(target, anchor) {
      insert(target, h1, anchor);
      append(h1, text0);
      append(h1, text1);
      append(h1, text2);
      current = true;
    },

-    p(changed, ctx) {
-      if (changed.name) {
-        setData(text1, ctx.name);
-      }
-    },
+    p: noop,

    i(target, anchor) {
      if (current) return;
      this.m(target, anchor);
    },

    o: run,

    d(detach) {
      if (detach) {
        detachNode(h1);
      }
    }
  };
}

...since there's no way name could change.

Stretch goal: let name should be hoisted out of the instance altogether:

function create_fragment($$, ctx) {
  var h1, text0, text1, text2, current;

  return {
    c() {
      h1 = createElement("h1");
      text0 = createText("Hello ");
-      text1 = createText(ctx.name);
+      text1 = createText(name);
      text2 = createText("!");
    },

    m(target, anchor) {
      insert(target, h1, anchor);
      append(h1, text0);
      append(h1, text1);
      append(h1, text2);
      current = true;
    },

-    p(changed, ctx) {
-      if (changed.name) {
-        setData(text1, ctx.name);
-      }
-    },
+    p: noop,

    i(target, anchor) {
      if (current) return;
      this.m(target, anchor);
    },

    o: run,

    d(detach) {
      if (detach) {
        detachNode(h1);
      }
    }
  };
}

+let name = 'world';

This is what happens if it's a const instead. In other words, rather than determining a variable's reactivity based on const and let (which causes other bugs — #1917), it should be based on how the variable is actually used.

@Rich-Harris Rich-Harris added this to the 3.x milestone Jan 3, 2019
@evs-chris
Copy link
Contributor

evs-chris commented Jan 12, 2019

Playing around, I extended TemplateScope to start tracking which names are mutated, which allows code that sets up an update to check its deps for known mutation before doing so. The first pass is quite rough, and there are a fair number of test failures. The thing that jumped out most for me though, were the component prop binding test failures. What happens here with <Hello bind:name />?

@evs-chris
Copy link
Contributor

So it turns out I was confusing myself (not hard to do) with a scriptless component. With that cleared up, I think tracking mutation along the template stack will probably work out ok, so I'll keep poking at it to see what shakes out.

evs-chris added a commit to evs-chris/svelte that referenced this issue Jan 13, 2019
Rich-Harris added a commit that referenced this issue Jan 13, 2019
First pass at tracking mutation to avoid unnecessary update code - #1952
@laurentpayot
Copy link

laurentpayot commented Jan 15, 2019

In other words, rather than determining a variable's reactivity based on const and let (which causes other bugs — #1917), it should be based on how the variable is actually used.

+1 as for CoffeeScript users (I'm a proud one) transpiled code only uses var instead of let or const.

Rich-Harris added a commit that referenced this issue Jan 19, 2019
hoist vars and lets that don't change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants