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

Replacing 'state' with 'context' #1122

Closed
Rich-Harris opened this issue Jan 20, 2018 · 1 comment
Closed

Replacing 'state' with 'context' #1122

Rich-Harris opened this issue Jan 20, 2018 · 1 comment

Comments

@Rich-Harris
Copy link
Member

Context created by each and await blocks is reflected in additional function parameters, as seen here:

for (var i = 0; i < cats.length; i += 1) {
  each_blocks[i] = create_each_block(state, cats, cats[i], i, component);
}

// later...
function create_each_block(state, cats, cat, cat_index, component) {
  // ...
}

Passing cats[i] and i as arguments, and appending to those arguments for each new context, may not be the best idea. I think it probably makes it harder to extract certain chunks of code out into reusable helpers.

I'd like to explore doing this instead:

-function create_main_fragment(state, component) {
+function create_main_fragment(component, context) {
  var h1, text_1, ul;

-  var cats = state.cats;
+  var cats = context.cats;

  var each_blocks = [];

  for (var i = 0; i < cats.length; i += 1) {
-    each_blocks[i] = create_each_block(state, cats, cats[i], i, component);
+    each_blocks[i] = create_each_block(component, assign({}, context, {
+      cat: cats[i],
+      cat_index: i
+    }));
  }

  return {
    c: function create() {
      h1 = createElement("h1");
      h1.textContent = "Cats of YouTube";
      text_1 = createText("\n\n");
      ul = createElement("ul");

      for (var i = 0; i < each_blocks.length; i += 1) {
        each_blocks[i].c();
      }
    },

    m: function mount(target, anchor) {
      insertNode(h1, target, anchor);
      insertNode(text_1, target, anchor);
      insertNode(ul, target, anchor);

      for (var i = 0; i < each_blocks.length; i += 1) {
        each_blocks[i].m(ul, null);
      }
    },

-    p: function update(changed, state) {
-      var cats = state.cats;
+    p: function update(changed, context) {
+      var cats = context.cats;

      if (changed.cats) {
        for (var i = 0; i < cats.length; i += 1) {
+        var each_context = assign({}, context, {
+          cat: cats[i],
+          cat_index: i
+        });
          if (each_blocks[i]) {
-            each_blocks[i].p(changed, state, cats, cats[i], i);
+            each_blocks[i].p(changed, each_context);
          } else {
-            each_blocks[i] = create_each_block(state, cats, cats[i], i, component);
+            each_blocks[i] = create_each_block(component, each_context);
            each_blocks[i].c();
            each_blocks[i].m(ul, null);
          }
        }

        for (; i < each_blocks.length; i += 1) {
          each_blocks[i].u();
          each_blocks[i].d();
        }
        each_blocks.length = cats.length;
      }
    },

    u: function unmount() {
      detachNode(h1);
      detachNode(text_1);
      detachNode(ul);

      for (var i = 0; i < each_blocks.length; i += 1) {
        each_blocks[i].u();
      }
    },

    d: function destroy() {
      destroyEach(each_blocks);
    }
  };
}

// (4:1) {{#each cats as cat}}
-function create_each_block(state, cats, cat, cat_index, component) {
-  var li, a, text_value = cat.name, text, a_href_value;
+function create_each_block(component, context) {
+  var li, a, text_value = context.cat.name, text, a_href_value;

  return {
    c: function create() {
      li = createElement("li");
      a = createElement("a");
      text = createText(text_value);
      this.h();
    },

    h: function hydrate() {
      a.target = "_blank";
-      a.href = a_href_value = cat.video;
+      a.href = a_href_value = context.cat.video;
    },

    m: function mount(target, anchor) {
      insertNode(li, target, anchor);
      appendNode(a, li);
      appendNode(text, a);
    },

-    p: function update(changed, state, cats, cat, cat_index) {
-      if ((changed.cats) && text_value !== (text_value = cat.name)) {
+    p: function update(changed, context) {
+      if ((changed.cats) && text_value !== (text_value = context.cat.name)) {
        text.data = text_value;
      }

-      if ((changed.cats) && a_href_value !== (a_href_value = cat.video)) {
+      if ((changed.cats) && a_href_value !== (a_href_value = context.cat.video)) {
        a.href = a_href_value;
      }
    },

    u: function unmount() {
      detachNode(li);
    },

    d: noop
  };
}

(Note that cat_index is unused, and we could probably eliminate it altogether — probably easier with context than with function args.)

This does result in slightly larger code for the example above (1862 from 1786 bytes minified, 591 from 576 zipped, a 4%/2.5% increase), and it means extra allocations for all the child contexts (though maybe we could reuse child contexts?). But if it does mean that code can be reused more effectively (e.g. for diffing keyed lists) then it would probably pay for itself quite quickly. (Perf-wise too, since functions would warm up quicker in theory.)

My hunch is that this would be a step towards being able to offer different compilation modes, such as to Glimmer-style bytecode.

Rich-Harris added a commit that referenced this issue Feb 11, 2018
Rich-Harris added a commit that referenced this issue Feb 11, 2018
Rich-Harris added a commit that referenced this issue Feb 23, 2018
Rich-Harris added a commit that referenced this issue Feb 24, 2018
@Rich-Harris
Copy link
Member Author

Implemented in 1.56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants