Keyed VDom #141

zaceno · 2017-03-07T23:31:14Z

Allow instructing the patch-function what dom nodes to reuse in update, via

h(tag, {_key: '...', ...}, [])

codecov · 2017-03-07T23:33:27Z

Codecov Report

Merging #141 into master will decrease coverage by 5.67%.
The diff coverage is 80.7%.

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage     100%   94.32%   -5.68%     
==========================================
  Files           4        4              
  Lines         160      194      +34     
  Branches       39       47       +8     
==========================================
+ Hits          160      183      +23     
- Misses          0       11      +11

Impacted Files	Coverage Δ
src/app.js	`92.76% <80.7%> (-7.24%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 305a2a3...31aca65. Read the comment docs.

zaceno · 2017-03-07T23:34:17Z

Solves #117 (among many other important use-cases)

Here is the code pen from that issue, demonstrating how it solves the problem.

http://codepen.io/zaceno/pen/dvOmNr

(wierdly though, something makes it break in safari, although it works in chrome. Need to look more at that before mergeing)

zaceno · 2017-03-07T23:51:27Z

Here is a nother, simpler problem this PR solves.

http://codepen.io/zaceno/pen/ryWddK

To demonstrate the problem, click the update button and notice how the images reload.
Now uncomment line 17 (_key: 'somekey') and try again. Notice that because the photo-gallery is rendered with the same _key property, the patch function knows to reuse it's dom-node, rather than replace it with the "updating..." message (and then create a new image-gallery node from scratch)

leeoniya · 2017-03-07T23:53:59Z

keyed nodes solve many problems, broadly summarized here:

#132 (comment)

farism · 2017-03-08T00:37:38Z

It is definitely something that will be needed in some form or another for long term performance reasons.

I haven't looked at the implementation, but the first thing that jumps out at me is why the leading _ in _key?

leeoniya · 2017-03-08T00:46:03Z

keys rarely help performance, and if implemented inefficiently can reduce performance. the overhead of internally searching & managing keys usually offsets the gains of having more optimal dom ops. the point of keys is to prevent unintended dom node reuse and accidental busting of any intermittent dom state that's not explicitly managed by the vdom lib.

the _ prefix is to discern vars used by the vdom lib itself from real attributes applied to the elements.

farism · 2017-03-08T01:32:32Z

Great, thank you for the explanation @leeoniya

jorgebucaran · 2017-03-08T06:06:38Z

@zaceno In your example what is model.current? Is it a number, a string? There is no model schema to infer this so one needs to understand all the code to figure it out.

EDIT: Okay, I see it's a Number.

jorgebucaran · 2017-03-08T06:07:53Z

@leeoniya Could you review this PR? 🙏

jorgebucaran · 2017-03-08T06:33:58Z

I'm going to need everyone's help to review this PR. @farism @dodekeract @danigb @selfup @jamen @liadbiz 🙏 🙏

leeoniya · 2017-03-08T06:49:37Z

i can sanity check it tomorrow. keep in mind though, an optimal keyed algo is thoroughly described here:

https://github.com/localvoid/kivi/blob/master/lib/vnode.ts#L1284

zaceno · 2017-03-08T08:20:47Z

@jbucaran: in my example model.current and model.next are strings with URLs

zaceno · 2017-03-08T08:21:41Z

Amen to some review on this! At this stage it's mostly a proof of concept that it's doable. But certainly needs some community-scrutiny

zaceno · 2017-03-08T10:45:48Z

src/app.js

+
+        if (child && child.data && child.data._key && keyedChildren[child.data._key]) {
+          var keyed = keyedChildren[child.data._key]
+          element.appendChild(keyed.element)


Probably more effincient in most cases, if this line were: element.insertBefore(keyed.element, element.childNodes[i])

zaceno · 2017-03-08T13:29:45Z

There is an issue with #141 and css transitions. It seems that the way we remove and reinsert keyed elements in the dom somehow breaks css-transitions in Chrome and Safari.

In order to make my sliding images demo work, I have to add classes to the element after the patch function is run, i e in oncreate and onupdate lifecycle methods.

http://codepen.io/zaceno/pen/dvOmNr

... not sure if there is something we should do about that in the patch-function, or just call it a browser-bug and accept the workaround.

EDIT: to be clear: the patch function adds classes correctly when you just specify them in the data object normally. There is no regression in this regard. It's just that whatever in the browser that reacts to class-changes and makes the transition happen, looses tracking once we remove and reinsert the DOMNode. As far as I can tell.

UPDATE 2: I investigated how to solve this, so that we kan do transitions with keyed elements without the workaround in the above pen. It is possible, but I didn't add it to the PR because I don't believe anyone will like it.

Basically it involves deferring the element.setAttribute(name, value) call in setElementData (conditionally for only the class attribute). It's a little wonky but not terrible. However, in order to make sure the new class is set when the onupdate handler is called, onupdate must be deferred twice. ... and then of course all tests using class attributes must be updated to be async...

All that, just to work around what seems like quite arbitrary browser behavior (I'd even go so far as to call it a bug)!

~~In the interest of keeping hyperapp clean and light, I'm happy to continue using the workaround.~~

leeoniya · 2017-03-09T16:08:13Z

quick update, because i've been somewhat busy. i want to be clear about what i planned check for this PR, and if anyone gets to this first, go ahead and do it.

for obvious reasons, i don't have much interest in dedicating a lot of time to hyperapp. if i spot anything obvious that i've learned to avoid, i will certainly point it out, but i will not be digging into the source here to validate that all things are coded optimally and without errors - that job is painstakingly huge, and i've already gone through it all in domvm via several vdom engine rewrites.

that being said, here's an easy way to figure out whether this PR does what it's supposed to from external testing alone:

create a list of 5 sibling nodes via hyperapp, each should have a textContent of a different color like "red". each should also be _keyed by the same value.
use vanilla dom to add a .style.color = "red" to match the created nodes' textContent.
return various templates in hyperapp that represent the following mutations:
a. remove first node
b. remove last node
c. remove a node from middle
d. swap 2 adjacent middle nodes
e. swap first and last nodes
f. insert a node in front
g. insert a node at end
h. insert a node in middle
i. insert 2 disjoint nodes in middle.
j. remove 2 disjoint nodes from middle.
after each "redraw", iterate the DOM and ensure that the .textContent matches .style.color.
validate which DOM operations were performed to achieve the needed dom state. you can use mutationObservers for this or you can use a lib i made for testing this in domvm [1]. You can see how it's used in domvm's regression tests [2].

if the above tests all pass, then i'd give this impl a thumbs up. I don't have a timeline for when i can get around to this, life is quite hectic these days, sorry.

[1] https://github.com/leeoniya/domvm/blob/2.x-dev/test/dominstr.js
[2] https://github.com/leeoniya/domvm/blob/2.x-dev/test/index.html

zaceno · 2017-03-09T21:54:11Z

@leeoniya thanks for that excellent suite of tests. I couldn't help myself but started checking my implementation against the suite you recommended (even though I know self-verification is not real verification) -- and it did uncover bugs in the implementation that I will start looking at now.

So there's no rush for anyone to verify this just yet (unless you're curious what the bugs were, and interested in fixing them, of course ;) )

leeoniya · 2017-03-09T22:24:04Z

(even though I know self-verification is not real verification)

that would imply that developers cannot be trusted to write unit tests for their own code 😆

zaceno · 2017-03-09T22:24:17Z

Ok, so the problem was the element.appendChild... line I commented on yesterday. It wasn't inefficcient -- it was wrong ;) After fixing that, @leeoniya 's tests all pass.

To "prove" it, here's a code-pen that cycles through the tests. One each second so that you can follow them visually.

http://codepen.io/zaceno/pen/VppvBg?editors=0011

If at any point the element.style.color doesn't match the element.textContent, the console should shout at you. Fortunately it doesn't :)

... Again, I prove nothing by verifying my own code of course. But consider it a green light to proceed with more verification & reviewing.

zaceno · 2017-03-09T22:27:01Z

@leeoniya ... hehe, you make a good point. But then I at least hope someone looks at my tests ;)

leeoniya · 2017-03-09T22:29:44Z

Again, I prove nothing by verifying my own code of course. But consider it a green light to proceed with more verification & reviewing.

no implementation is bug free. but code that passes a non-trivial set of smell tests and has automated regression tests can be merged. if all existing tests pass and no new tests fail, that's the best anyone can do. the only enemy is not enough existing tests - there are never enough.

if domvm's tests have even 50% code coverage, i would be shocked - and it has 500+ assertions and thousands LOC worth of testing code. it's a major pain in the ass to write them all, but that's what separates toy MVP projects and production-grade libs you can trust not to break apps between releases and only improve with time. v2 was a major from-scratch rewrite of v1 but the tests ensured that essentially nothing broke [1].

[1] domvm/domvm#116

ngryman · 2017-03-10T00:35:08Z

IMHO _key should be key (https://github.com/Matt-Esch/virtual-dom/tree/master/virtual-hyperscript#special-properties-in-h).

zaceno · 2017-03-10T07:53:54Z

@ngryman -- fine with me. Makes sense considering onremove et c also have no special prefix.

AutoSponge · 2017-03-11T16:30:04Z

@jbucaran it made more sense in my head...

To allow for keys to work and allowing real elements to be held by reference in the model (my PR), the setElementData process would need to set isSameNode to the element (since this implementation only knows how to use keys based on node.data.key which would not exist on an element). The isSameNode fn would return true when passed a vnode having the expected key.

So, if creating an element from a vnode with a key, also set the isSameNode method on the element with a fn that can match a vnode or element. I'm just anticipating one of these landing before the other...

jorgebucaran · 2017-03-11T16:34:36Z

@AutoSponge Haha okay.

I am currently working on my own keys implementation based in #141. Like you say I am not sure if it will be compatible with #151, but on the other hand, I still haven't quite understood what's the purpose of that PR. Don't take that as criticism please, on the contrary, I just need more time or perhaps if you have time you can try ELI5 to me again.

AutoSponge · 2017-03-11T17:56:51Z

@jbucaran no problem! I'll add explanation to #151

jorgebucaran · 2017-03-13T17:12:36Z

@zaceno 👋 Is this done or you planning tweaking something more?

zaceno · 2017-03-13T19:19:42Z

@jbucaran, Hi sorry for the silence :)

No, it's not complete. I am working on a rewrite where I do removeChild(...) only when absolutely necessary (to fix the css-transition issue, among other things). It's going to make the diff bigger, but it will be a more complete solution.

I didn't get as much time as I had hoped this weekend, but I am working away at it little by little, and hope to have an update to this PR soon. By the end of the week at the latest.

UPDATE: It's looking promising. All unit tests -- and all but a couple of @leeoniya:s smoke tests above -- are passing. Just need to tinker a little more to get it all working (+ some polish to make it more presentable) Then I will submit an update. But not tonight... it's late here now :)

zaceno · 2017-03-14T22:58:51Z

@jbucaran So now I believe this solution is complete. It ended up bigger than I hoped, and perhaps touching more of the code than you would like. But it works.

(And because I don't needlessly remove & reinsert dom-nodes any longer, it does not break css transitions.)

Here are updated versions of all the code pens I've mentioned before:

@leeoniya s suite of smoke tests visualized here: http://codepen.io/zaceno/pen/wJqmjY
...notice that the console is silent. Proof that the keyed vdom solution does what it should

My css transitions slideshow with images: http://codepen.io/zaceno/pen/WpEJQV
... no flickering reloading images. See it break when you comment out the key: ... lines.

My css transitions slideshow with iframes: http://codepen.io/zaceno/pen/oZeqRG
... no reloading iframes! See it break when you comment out the key: ...lines.

the image gallery example: http://codepen.io/zaceno/pen/bqrvQO
... notice how when you press the update button, the gallery images don't reload (as intended). but if you comment out the key: ... line, then they do reload when the update button is pressed (a problem keys solve)

AutoSponge · 2017-03-14T23:37:14Z

I solved a similar problem in my PR #151. The problem comes when replacing an element with a sibling element instead of just removing the preceding sibling.

I simply perform the remove then, remove the reference from the oldNode.children array then update the array's length, oldLen. If oldNode.children held a reference to the element it created the first time it went through render, you could perform the same check I'm doing (since I'm looking at real dom nodes and using .compareDocumentPosition).

If you want, I can try to add that to my PR and test your original demos against it. I just need your original demo code if you have it (unless I just need to remove the key from all the nodes to do the test, in which case I can do that).

zaceno · 2017-03-14T23:50:20Z

@AutoSponge, if I get what you're saying, I think a keyed vdom covers more cases than just the one you're describing, but the case you're describing is the problem I initially had with my iframe/image slidedshow demos.

I know my case can be solved with something simpler than keyed vdoms (I had a PR out before this one, that solved it in a more minimal way) -- but I'm absolutely curious to see your solution (I think I'd understand better what you're saying if I saw it anyway), so go for it! :)

That said, because keyed vdoms cover more cases (specifically: when you need to reuse but reorder dom elements), I'm hoping for that feature in hyperapp rather than a less complete solution.

Hope I understood you correctly! If I didn't, it wasnt intentional :)

AutoSponge · 2017-03-16T16:47:56Z

@zaceno I started this work but I'm not quite done. As I worked, I thought id should be used as the key instead of key since (a) it should already be unique and (b) it's easy to get the list of "keyed" elements with parent.querySelectorAll("[id]").

I could continue to work on this implementation but don't let me hold up acceptance. My changes in #151 are likely to fold in easily since it's not that big.

zaceno · 2017-03-16T20:14:33Z

@AutoSponge Cool, thanks for sharing that idea! The thing about keys vs ids is, though, is that while id's must be unique within the entire document, keys only need to be unique among sibling-nodes. And there are cases where you might want to have the same key in multiple places in a document. Like if you have multiple of the same components on a page.

By all means keep working, and don't hold your breath on this PR! I hope its not long before we can have keyed patching in hyperapp, but this may not be the implementation that makes it.

@jbucaran expressed hesitance on the slack-channel, against both the performance-degradation and increased bundle size this brings with it. So to make this happen I'll need to both optimize and condense it. But frankly, right now I'm not sure I can do any better, so I'm not feeling that hopeful :(

Thankfully @jbucaran is working on an implementation of his own, and I hope he fares better :)

zaceno · 2017-03-17T09:22:01Z

Given the state of this PR, I want to express my purely personal and subjective opinion that: if no better solution (more performant, smaller) can be found in the near term, this PR should be merged anyway.

There are several use cases where this functionality (not necessarily my implementation) is absolutely critical. Hyperapp needs this if it wants to be a "general purpose" frontend framework.

Despite the shortcomings of my implementation in terms of size and performance, it does work. Once it's in, we can and should continue looking for optimizations.

... that is of course only if no better implementation can be found in the short term. I'm still willing to wait for @jbucaran 's solution (or anyone else currently having a go at it)

jorgebucaran · 2017-03-17T10:04:08Z

@zaceno Thanks for your work on this PR! I agree with you this is an important feature we need to ship ASAP. I hope I can dedicate some good time to this on the weekend.

zaceno · 2017-03-18T01:03:25Z

So I made some adjustments and brought back the batch-delete. (thanks @jbucaran for brilliantly reminding me that dom nodes don't have to be detached from the document to be inserted!!) I also tried to refactor it into a hopefully more compressible way.

I wasn't able to get the benchmark thing running, so I havent been able to check performance. But hopefully this should be much better in performance, and a little smaller as well.

jorgebucaran · 2017-03-27T03:35:23Z

src/app.js

-      element = parent.appendChild(createElementFrom(node))
-
-    } else if (node === undefined) {
+  function batchRemove (parent, element, node) {


@zaceno We need to find another way to do this. onRemove should be called right before the node is deleted, so we need to save the onRemove handler in the batch array somehow.

I don't se any problem batching the onRemove-handler, BUT even before this PR, the onRemove handler was deferred, meaning it gets was called after the node is removed. So... do you want me to fix that behavior as well, or leave it as it is?

I pushed it to master a while ago, it's like 10 bytes smaller so...

jorgebucaran · 2017-03-27T03:35:42Z

src/app.js

+    if (oldNode === undefined) {
+      var n = createElementFrom(newNode)
+      parent.insertBefore(n, element)
+      element = n

    } else if (


@zaceno This logic changed in a recent commit on master.

If you're referring to the "insert app directly into document.body" merge, then yes. This code has been updated specifically to adapt to that (to return the new element of a patch)

jorgebucaran · 2017-03-27T03:36:34Z

src/app.js

+  }
+
+  function patch (parent, element, oldNode, newNode) {
+    if (oldNode === undefined) {


If element is undefined/null, the new node will be appended to parent, which is what we want, but if element is not null, then the new element will be inserted on top of element. Is this correct?

Yes, that is correct

jorgebucaran · 2017-03-27T03:37:00Z

src/app.js

-      updateElementData(element, node.data, oldNode.data)
+      var n = createElementFrom(newNode)
+      parent.replaceChild(n, element)
+      element = n


jorgebucaran · 2017-03-27T03:39:11Z

src/app.js

+    // We need to index all keyed nodes in oldChildren in order to
+    // distinguish new keyed nodes from nodes to reuse in newChildren
+    var keyedChildren = {}
+    for (var i = 0; i < oldNode.children.length; i++) {


Would using a no-op branch instead of continue mean less bytes?

I'll try it :)

jorgebucaran · 2017-03-27T03:43:11Z

src/app.js

+        isKeyed(newChild) &&
+        cache[newChild.data.key]
+      ) {
+        var c = cache[newChild.data.key]


Use var i unless impossible, it compresses better.

You mean var i compresses better than var c? Is that because var i is already used in a lot if places?

Either way, I'll fix that, no problem.

jorgebucaran · 2017-03-27T03:43:21Z

src/app.js


-      for (var i = 0; i < len || i < oldLen; i++) {
-        patch(element, element.childNodes[i], oldNode.children[i], node.children[i])
+      } else if (oldChild === undefined) {


jorgebucaran · 2017-03-27T03:44:16Z

src/app.js

-    return element
+    for (var key in cache) {
+      var unused = cache[key]
+      batchRemove(element, unused.element, unused.node)


What happens if a node is already scheduled for remove by patch, we would end up batching the same node twice.

I'm pretty sure that can't happen. Unkeyed nodes get scheduled for removal as we iterate over them. Keyed nodes are put in the cache, and scheduled for removal after we see we haven't used them.

jorgebucaran · 2017-03-27T03:47:37Z

test/app.test.js

+        })
+      })
+
+      it("fires onremove for ONLY the not-reused keyed nodes", done => {


🤔 It's gonna be more work for me later to clean these tests, so please remove them all from this PR. I'll write tests myself later. 🙏

Ok, I'll remove them. But can we wait until it's definitely time to merge? Because they're pretty useful for knowing I don't break stuff when I fix merge-conflicts.

Of course, of course.

jorgebucaran · 2017-03-27T03:57:49Z

src/app.js

+        patchChild()
+
+      } else {
+        cache[oldChild.data.key] = {


Maybe smaller would be to map keys to an array and store the node at 0 and element at 1?

Hmm... not sure I follow. I'm happy to try it, but could you give me a code-snippet to explain more. What I'm envisioning is not smaller so I'm sure I'm misunderstanding :)

zaceno · 2017-03-27T23:34:16Z

Latest adjustments brings the total compressed bundle size down to 1954 bytes.

jorgebucaran · 2017-03-28T08:08:12Z

src/app.js

@@ -254,93 +254,66 @@ export default function (app) {
  function patchChildren(element, oldNode, newNode) {
    // We need to index all keyed nodes in oldChildren in order to
    // distinguish new keyed nodes from nodes to reuse in newChildren
-    var keyedChildren = {}
+    var oldChildren = []


@zaceno Nice!

jorgebucaran · 2017-03-28T08:09:14Z

src/app.js


-      var len = node.children.length
-      var oldLen = oldNode.children.length
+    var isKeyed = function (node) {


I think there should be no var x = function(){..}. This pattern only adds more bytes.

jorgebucaran · 2017-03-28T08:10:16Z

src/app.js

+    var cache = {}
+
+    while (
+      newIndex < newNode.children.length ||


@zaceno We should try to access stuff only once. len, oldLen, oldNodeData, etc.

jorgebucaran · 2017-03-28T08:10:54Z

src/app.js

+        patch(element, oldChildren[oldIndex][0], null, newChild)
+        newIndex++
+
+      } else if (newChild == null || (!isKeyed(oldChild) && isKeyed(newChild)) ) {


@zaceno Perhaps you can store the oldKey and newKey in a variable, then just check oldKey != null to see if a node is keyed or not.

I tried that, but that change actually added 11 bytes. Either way I'll leave the change in the upcoming commit, so that you can see the result and perhaps help me find a shorter way to write it.

Interesting! Thanks for letting me know that :)

I managed to compensate for those 11 bytes with some other changes (in the upcoming commit)

jorgebucaran · 2017-03-28T08:11:20Z

src/app.js

    }

-    return element
+    for (var key in cache) {
+      batchRemove(element, cache[key][0], cache[key][1])


jorgebucaran · 2017-03-28T08:12:28Z

src/app.js

+    var oldChildren = []
+    var oldKeys = {}
+    for (var i = 0; i < oldNode.children.length; i++) {
+      oldNode.children[i].data &&


@zaceno Let's if instead of && here. It adds no value since uglifyjs will do and undo with this code anyway.

jorgebucaran · 2017-03-28T13:40:36Z

src/app.js

-      var len = node.children.length
-      var oldLen = oldNode.children.length
+    var isKeyed = function (node) {
+      return node.data && node.data.key && oldKeys[node.data.key]


@zaceno This means keys can't be numbers, specifically the number 0. I don't have an issue with that, but is that what we want?

Actually I think we can have numbers. The number 0 as well. Not sure exactly how various browsers handle this, but in v8 at least (just popped open my node console and tested this)

> var a = {} undefined > a[0] = 'foo' 'foo' > a {'0': 'foo'} > a[0] 'foo'

Sure, it will cast numbers to strings for the internal representation of the hash keys. But as long as you are not using a mix of numbers and stringed numbers to key your children (and that is a really unlikely scenario), it should work without conflict

To clarify: the problem we still have is that the keys 0 and '0' (or 42 and '42') will be considered identical and result in undefined behavior if used for different sibling nodes. But I think that is an unlikely scenario. Either you are using strings, or you are using only numbers. Probably. If you are using a mix of strings and numbers, then at least your strings are probably not strings of numbers (e g '42')

@leeoniya Which one?

@zaceno If node.data.key is the number 0, then the above condition will result in iskeyed returning false.

What I meant was, if we are going to support keys as Numbers, then the smallest check that works is using null.

right, the condition will fail for all falsey values, which is wrong; all strings and numbers are valid keys. it's not a hash issue, which wull stringify all object keys.

@leeoniya & @jbucaran: good points. Fixed in upcoming

zaceno · 2017-03-29T09:03:50Z

Current bundle size: 1935 bytes

jorgebucaran · 2017-03-29T11:20:36Z

@zaceno Now that #172 is out there, perhaps you can help there? We've been through this over Slack arleady. I am super thankful for all the work that went into this PR and all your feedback since you created #117 and I look forward more from you after this and from now on too! 👋

zaceno · 2017-03-29T12:54:21Z

@jbucaran Of course! I've already left a couple of comments and am continuing to review it. As far as code style goes, I haven't got a lot to say since it's your project :). But I am going to pull it down and run it through some tests later tonight.

Anyhow until 172 is merged, I'll try to keep 141 in parity. If only for the sake of personal pride (Since you mentioned it would be pulled into the history with 172 on top of it, I don't want my name attached to something half-assed ;) ) Removing the deferred is one such parity-keeping effort. It was a very small effort really -- took all of 5 minutes, so...

Sometimes it may be useful to reuse dom nodes -- for performance reasons or to preserve browser state on them (for instance, an image loaded via the src attribute). This allows you to instruct the vdom-patcher which DOM-nodes to reuse, through a key attribute on vdom-nodes

zaceno · 2017-03-30T05:58:26Z

Cleaned up the PR. Removed tests (as requested) and squashed to a single commit.

jorgebucaran · 2017-03-30T06:40:29Z

@zaceno Cool! I benchmarked my implementation and it's not as bad as I thought, 1.72, but I am sure we can do better.

Cache the old keys and old element lists might make a difference. Cache everything, we can even memoize toLowerCase. Also, going to try a different approach when removing stuff.

It would be great if you could review my PR more and suggest other changes or how to use some of the features from your PR, since they are both pretty similar after all.

It's not so much about making it smaller, but how to make it faster now, we can figure out how to make it smaller later.

zaceno · 2017-03-30T08:43:49Z

@jbucaran Actually I suspect that our attempts to make it smaller have led to poorer performance. I've been hearing a lot of chatter lately about how specific declarative checks like if (typeof foo === undefined) run much faster than if (!foo) or if (foo != null) -- because of the casting that isn't necessary in the first case. Here's an example of such chatter I've been hearing

preactjs/preact#610

(I'll duplicate this post over on your PR so the discussion can continue there)

jorgebucaran · 2017-04-03T08:20:05Z

@zaceno 🎉

zaceno mentioned this pull request Mar 7, 2017

Allow specifying nodes to insert #135

Closed

zaceno mentioned this pull request Mar 7, 2017

prevent images and iframes reloading on patch? #117

Closed

jorgebucaran self-requested a review March 8, 2017 01:41

jorgebucaran added enhancement New feature or request FUTURE labels Mar 8, 2017

jorgebucaran mentioned this pull request Mar 8, 2017

does hyperapp's virtual dom support keyed children? #132

Closed

jorgebucaran requested a review from FlorianWendelborn March 8, 2017 06:32

zaceno commented Mar 8, 2017

View reviewed changes

jorgebucaran mentioned this pull request Mar 11, 2017

Internal routing hook for <a href=/*> #130

Closed

jorgebucaran mentioned this pull request Mar 15, 2017

onclick firing on wrong elements #161

Closed

zaceno mentioned this pull request Mar 21, 2017

dangerouslySetInnerHTML? #170

Closed

jorgebucaran reviewed Mar 27, 2017

View reviewed changes

jorgebucaran reviewed Mar 28, 2017

View reviewed changes

jorgebucaran mentioned this pull request Mar 28, 2017

Introduce keys. #172

Merged

jorgebucaran merged commit 31aca65 into jorgebucaran:master Apr 3, 2017

Keyed VDom #141

Keyed VDom #141

Conversation

zaceno commented Mar 7, 2017 • edited by jorgebucaran Loading

codecov bot commented Mar 7, 2017 • edited Loading

Codecov Report

zaceno commented Mar 7, 2017 • edited Loading

zaceno commented Mar 7, 2017

leeoniya commented Mar 7, 2017

farism commented Mar 8, 2017

leeoniya commented Mar 8, 2017 • edited Loading

farism commented Mar 8, 2017

jorgebucaran commented Mar 8, 2017 • edited Loading

jorgebucaran commented Mar 8, 2017

jorgebucaran commented Mar 8, 2017

leeoniya commented Mar 8, 2017

zaceno commented Mar 8, 2017

zaceno commented Mar 8, 2017

Choose a reason for hiding this comment

zaceno commented Mar 8, 2017 • edited Loading

leeoniya commented Mar 9, 2017 • edited Loading

zaceno commented Mar 9, 2017

leeoniya commented Mar 9, 2017

zaceno commented Mar 9, 2017 • edited Loading

zaceno commented Mar 9, 2017

leeoniya commented Mar 9, 2017 • edited Loading

ngryman commented Mar 10, 2017

zaceno commented Mar 10, 2017

AutoSponge commented Mar 11, 2017 • edited Loading

jorgebucaran commented Mar 11, 2017

AutoSponge commented Mar 11, 2017

jorgebucaran commented Mar 13, 2017

zaceno commented Mar 13, 2017 • edited Loading

zaceno commented Mar 14, 2017

AutoSponge commented Mar 14, 2017 • edited Loading

zaceno commented Mar 14, 2017

AutoSponge commented Mar 16, 2017

zaceno commented Mar 16, 2017

zaceno commented Mar 17, 2017

jorgebucaran commented Mar 17, 2017

zaceno commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaceno commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaceno Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeoniya Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

zaceno commented Mar 7, 2017 •

edited by jorgebucaran

Loading

codecov bot commented Mar 7, 2017 •

edited

Loading

zaceno commented Mar 7, 2017 •

edited

Loading

leeoniya commented Mar 8, 2017 •

edited

Loading

jorgebucaran commented Mar 8, 2017 •

edited

Loading

zaceno commented Mar 8, 2017 •

edited

Loading

leeoniya commented Mar 9, 2017 •

edited

Loading

zaceno commented Mar 9, 2017 •

edited

Loading

leeoniya commented Mar 9, 2017 •

edited

Loading

AutoSponge commented Mar 11, 2017 •

edited

Loading

zaceno commented Mar 13, 2017 •

edited

Loading

AutoSponge commented Mar 14, 2017 •

edited

Loading

zaceno Mar 28, 2017 •

edited

Loading

leeoniya Mar 28, 2017 •

edited

Loading

zaceno commented Mar 29, 2017 •

edited

Loading

jorgebucaran commented Mar 30, 2017 •

edited

Loading