Support roots with existing children (#128) #140

ngryman · 2017-03-07T13:43:28Z

Here it is :)

FlorianWendelborn · 2017-03-07T13:46:18Z

Does this also work when the third-party element vanishes or another one gets added at runtime?

codecov · 2017-03-07T13:48:14Z

Codecov Report

Merging #140 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #140   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         163    165    +2     
  Branches       40     40           
=====================================
+ Hits          163    165    +2

Impacted Files	Coverage Δ
src/app.js	`100% <100%> (ø)`	⬆️

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 7d2a96e...1010885. Read the comment docs.

ngryman · 2017-03-07T13:58:16Z

@dodekeract I don't think so, my bad...
We should track the node element instead of the node index and compute the index dynamically. I've tried this approach but couldn't make it work with the router which basically changes the node element. I have to go now, but I'll figure this out in the afternoon.

ngryman · 2017-03-07T13:59:01Z

Also it would be nice to expose somehow internal app functions in order to unit test them.

jorgebucaran · 2017-03-07T14:15:31Z

@ngryman Awesome! I still need to run this thru the benchmarks however. Patch is heavily used and we don't want worse performance just to have the ability to insert an app in document.body right? :)

FlorianWendelborn · 2017-03-07T14:28:01Z

@jbucaran I don't think this issue is related to document.body.

leeoniya · 2017-03-07T14:40:12Z

accessing childNodes and using indexOf in a hot function is gonna be a perf killer...although doing it only for root might be ok.

jorgebucaran · 2017-03-07T15:03:46Z

@dodekeract Do you have an failing example to back that up?

FlorianWendelborn · 2017-03-07T15:35:55Z

@jbucaran #128 (comment)

jorgebucaran · 2017-03-07T15:37:12Z

@dodekeract I see what you mean. The issue is embedding the app in an element that is not empty.

Why would anyone do that anyway?

FlorianWendelborn · 2017-03-07T15:38:27Z

@jbucaran Only relevant for document.body since browser plugins may add elements there. That's the reason why React doesn't support rendering to <body>.

ngryman · 2017-03-07T15:39:07Z

@leeoniya @jbucaran A cleaner solution would be perhaps to invoke a patch outer function that executes some specific code for the root and then call recursively a patch inner function.

jorgebucaran · 2017-03-07T15:39:48Z

@dodekeract What do other libraries do btw?

@ngryman Do we have to run this code on every patch or just once?

FlorianWendelborn · 2017-03-07T15:42:02Z

@jbucaran React just decided to not support <body> because of this. I don't know what happens if the root has other elements, but I'd assume that react removes them.

ngryman · 2017-03-07T15:43:51Z

@dodekeract If I remember well React warns the user about using document.body but supports it.
@jbucaran Basically, the patched code would be executed only the first time the render function is invoked, and each time a route is matched when using the router plugin.

jorgebucaran · 2017-03-07T15:46:35Z

@ngryman Then why is the code inside patch? Maybe outside render, and change render to take an index argument too?

The router...

ngryman · 2017-03-07T15:47:52Z

@jbucaran Because I need the actual DOM element which is created inside patch. But there is probably a cleaner way to do so, I'm trying to understand the code at the same time :)

FlorianWendelborn · 2017-03-07T16:18:59Z

@ngryman Is that new? I remember that this was one of the few things I disliked when starting with react. Could have changed "recently".

jorgebucaran · 2017-03-07T16:22:03Z

JSX as well as Hyperx don't allow you to do stuff like this:

<h1>Hi</h1>
<p>Hello.</p>

Why should the vDOM engine allow you to embed an app in a container with children already in it?

ngryman · 2017-03-07T16:28:02Z

First of all some terminology, let's define the host as the existing dom element that receives the root node of the application.

Here is an unelegant solution, but it highlights what could be done: treat host operations separately from the root and all of its children.

ngryman · 2017-03-07T16:29:23Z

@jbucaran That's a different problem. Any jsx markup needs one root element to be inserted into any host element. I think we lack some terminology where we differentiate those 2 elements.

ngryman · 2017-03-07T16:37:10Z

@jbucaran If you agree with this I could factorize this and add associated tests.

jorgebucaran · 2017-03-07T16:42:30Z

@ngryman Go for it and let's see! 🙏

ngryman · 2017-03-07T17:25:52Z

@jbucaran Here is the idea.

After refactorization, we end up with just 1 more if for create, replace and update operations which should be perfectly fine. You can still bench it to be 100% sure. But I'm pretty sure there will be no difference. If there is, encapsulating each operation in its own function would let v8 and brothers optimize each one.

I just need to add one more test to check if it works well when mutating the root node outside hyperapp. If you have an example on how to test this it would be awesome. I'll finish this tomorrow :)

farism · 2017-03-07T19:54:12Z

I'm on the fence about this feature. First gut instinct is that it is unnecessary as there are current easy workarounds (e.g. creating a placeholder element inside of the targeted root element and targeting that instead).

And I think for the most part, people coming from other view libraries are expecting mounting a DOM tree to completely replace the contents.

jorgebucaran · 2017-03-07T20:02:52Z

And I think for the most part, people coming from other view libraries are expecting mounting a DOM tree to completely replace the contents.

Can you mention at least one library that works like this?

leeoniya · 2017-03-07T20:07:34Z

domvm can replace/absorb (and will clear) a target placeholder via .mount(placeHolder, true). but regular .mount(container) will append the view's root node to the container. [1]

if you support absorbing a pre-existing root or placeholder, then it's reasonable to expect content to be replaced. however, i think that most people would expect the app's root node to be appended to the container they specify, at least by default.

[1] https://github.com/leeoniya/domvm#views

farism · 2017-03-07T21:58:05Z

@jbucaran Yes: React. Which is arguably the most familiar to masses.

Here's my opinion: hyperapp is not a full fledged vdom implementation. After trying other vdom implementations it was decided that it would be best to keep the footprint small and implement a custom vdom. Should this vdom have all of the capabilities of another implementation? If so, we should probably just be using another library, which has smart people maintaining it and interested in solving those domain problems.

In my eyes this project is more about the elm architecture than it is about giving people options. If anything, being restrictive on these types of edges cases is probably better than being open, as it coerces people into doing one thing consistently.

If this feature request is to easier enable people to mount components into a DOM tree without blowing away the existing DOM, then my suggestion is to just make a placeholder element at end (or beginning, or wherever you want, this technique is actually more flexible) and render into that. It seems to be a perfectly acceptable solution.

ngryman · 2017-03-08T10:22:37Z

@farism tldr I understand your opinion and would totally agree with you if the cost of this feature was actually high. But it's not.

hyperapp should be and stay KISS. But it doesn't mean that every edge case should be thrown away and not implemented. IMHO judging a feature implementation based on what others do and hypothetical complexity is not sufficient enough.

In fact many factors are involved:

Legitimity: Is is something useful?
Usage: How many users would it impact?
Stability: Does it resolve a bug?
Weight: Would it add weight to the library?
Performance: Will it have a negative impact on performance?
Maintenance: Will it increase code complexity?

I've taken the liberty to check what I think this feature is OK with. The only remaining item would potentially be the impact of this feature, which I agree with you is pretty low if we consider the current usage.
But I'm pretty sure I'm not the only one annoyed by this additional placeholder.
IMHO I would even say that asking the user to workaround because we don't want to handle this is not a solution, it's laziness. If for a given use case everybody has to work around, well there is a conception problem somewhere...

hyperapp exists to be an opinionated alternative to react. So why wouldn't it also be opinionated on this too?

Still, you have 5/6 items checked meaning that you have a pretty good score on this feature. And it's actually 4 loc... I see a huge benefit for a tiny cost here.

And I think for the most part, people coming from other view libraries are expecting mounting a DOM tree to completely replace the contents.

Well I don't expect this at all tbh. In fact, what is the benefit for the user to do so? I don't see it, I only see a benefit for the library author that don't want to mess with this.
Also, it's not because react sempai does it this way that it's the way. If people come from other libraries there is a reason: they are unsatisfied with some key points and expect different (opinionated) things.

Sorry if it was veeeery long, but it's an interesting discussion because it raises the question: is hyperapp ~= react + elm or hyperapp = vdom + elm ?

lukejacksonn · 2017-03-15T00:07:33Z

I haven't dug too deeply into the code around this issue so I am not entirely sure this is relevant but all the talk of document fragments got me thinking about range.createContextualFragment which I have always found more useful than createDocumentFragment . I'm sure there are some downsides (seeing as it is rarely ever employed) but some things I like about it are..

It works great with template literals
No need for wrapper elements
Browser support back to IE11 (not 10 I know)

const frag = template => {
  const range = document.createRange();
  range.selectNode(document.body);
  return range.createContextualFragment(template.trim());
};

const model = { name: 'Joe Bloggs' }
const view = (model) =>`
  <img src='https://unsplash.it/200'
  <h1>Welcome ${model.name}</h1>
  <button>Click Me</button>
`

const node = frag(view(model))
document.body.appendChild(node)

EDIT: http://codepen.io/lukejacksonn/pen/VpzBJy

jorgebucaran · 2017-03-15T07:00:01Z

@ngryman I found a simpler way (maybe).

One way to do it:

Have patch return the appended element (when oldNode === undefined) this guarantees we get the application root node inside render after patch returns.
Store it in another global variable; ignore all future returned nodes since we don't care anymore.
Again, in render, but before we call patch, calculate the correct index based in the stored root element.

To calculate the index I am using:

  function getElementIndex(element) {
    for (var i = 0; element = element.previousSibling; i++);
    return i
  }

I'm using previousSibling and not previousElementSibling, since we want to account for text nodes too.

That's it. Seems to work. Now I'm trying to see how to cache the index, so we don't have to run getElementIndex on every render. This should be okay.

ngryman · 2017-03-15T11:46:15Z

@lukejacksonn I didn't know createContextualFragment, that's some interesting stuff!

@jbucaran Yes right, that would remove a bunch of if. So basically you would store the new nodeElement outside patch.
Another idea to explore (and bench) would be to avoid the initial index lookup you proposed by accepting index to also be an element. So instead of searching in parent.childNodes you could simply patch the element itself. The only cost for this would be a var element = 'object' === typeof index ? index : parent.childNodes[index]

jorgebucaran · 2017-03-15T11:59:36Z

@ngryman I think I might have ended up with something similar to that. And I was able to get rid of the index too.

Now, I'm just passing the parent and the first child of the parent to patch. I cache the first child inside load (not render) so it only happens once when the app starts (which makes more sense).

The only changes are that both render and patch return an element (the one we cache inside load) and that we calculate the index of this element relative to app.root once in load like this:

for (var i = 0; element = element.previousSibling; i++);

Also, the default app.root is now document.body. 😄

ngryman · 2017-03-15T13:03:58Z

@jbucaran Sounds promising 👍

From what you describe, I just want ot make sure you cover:

document.body mutations outside hyperapp. Those mutations would invalidate the index that you only computed once in load. Handling those mutations would involve an index lookup each time render is called, to ensure it's up-to-date.
The router use case that basically replace your cached element with a new one. From what I understand it would be updated in render form the returned element of patch.

I think we would need your code to discuss this 🤓

jorgebucaran · 2017-03-15T13:17:51Z

@ngryman There's one use case that won't work for sure, if your root element changes. See the example below.

app({
  model: 0,
  actions: {
    add: model => model + 1
  },
  view: (model, actions) => {
    var el
    if (model === 0) {
      el = <h1 onClick={actions.add}>Small</h1>
    } else if (model === 1) {
      el = <h2 onClick={actions.add}>Smaller</h2>
    } else {
      el = <h3>Tiny</h3>
    }
    // return el // BAD
    return <code>{el}</code> // OK
  }
})

Not sure if an easy solution is possible. But I'm not too worried about it.

~~Yep, will definitely fail then. What would you suggest we do? Looking up the index in render will be expensive. Is this worth it?~~
Not sure if this is a problem. Router tests pass, but seems similar to the code example above. As long as the root element of the application is the same tag, it's okay.

FlorianWendelborn · 2017-03-15T15:44:15Z

@jbucaran I think if there is no relatively easy solution that works in all cases we shouldn't consider it. Having apps that sometimes break for users with browser addons vs having to use a <div> is clearly in favor of the <div> IMO.

jorgebucaran · 2017-03-15T15:53:12Z

@dodekeract ~~Ah, I see what you are saying. Bummer. In that case we need a different solution because my code doesn't work with elements added dynamically, unless we make render slow and we don't want that right.~~

jorgebucaran · 2017-03-15T16:24:31Z

@dodekeract @ngryman Wait, wait my bad. We are caching an element, not an index. We calculate the index only once, inside load, and then get app.root's first child, we discard the index then.

Even if app.root has elements appended (or inserted at the top) we start patching from the cached first child.

tl;dr it's okay.

jorgebucaran · 2017-03-16T05:00:00Z

@ngryman Like I said above, it works, but I found it fails in a strange way when the root node is an SVG element and the body is modified. 😓

Would you like to continue working on this feature? Perhaps using my idea as a base and as a different PR.

ngryman · 2017-03-16T10:11:55Z

@jbucaran Sure! I won't be able before this WE thought. Can you provide your code on some branch so I can start from here?

jorgebucaran · 2017-03-16T11:54:10Z

@ngryman Sure thing!

Would you mind joining the Slack room? 🙏

zaceno · 2017-03-21T20:08:23Z

I've been trying to catch up with what's going on here... Am I correct in understanding the following:

that the original ("4 LOC") implementation of this feature is no longer relevant?
we are exploring a different solution to the problem (based on caching the app's top element)?
... there is, as yet no code to play with (in a PR or codepen or whatever) for this new solution-in-progress?

jorgebucaran · 2017-03-22T00:31:19Z

@zaceno The code to play with is on this gist.

https://gist.github.com/jbucaran/fc3651391b45e417f4dc04e771a88b00

ngryman · 2017-03-22T09:01:08Z

@zaceno Yes this implementation works, or at least worked (hyperapp is evolving fast 🤓 ). And it's effectively 4 LOCs. We basically cache the topmost node element in order to patch it directly and thus avoiding conflicts with other external elements.

@jbucaran wanted to explore another way to do so in the provided gist.

I'm going to patch this PR given the idea of @jbucaran to return the element from patch.

ngryman · 2017-03-22T09:36:11Z

And here it is 😄

jorgebucaran · 2017-03-22T09:52:13Z

@ngryman Is it necessary to return the element at the end of patch? I think we only care about it when items are appended to the DOM, no?

BTW, how did you fix the issue I posted as a gif on Slack? 🤔

ngryman · 2017-03-22T10:24:19Z

@jbucaran Well it's only necessary when we append and replace the element. My guts tell me it's better to have one return point for the whole function in order to kiss and have a clear contract established. Plus it avoid any additional tests outside patch to check if a return value is present.

I don't see your codepen link in Slack history, you told me about a svg clock if I remember. That said, I added 2 additional tests that covers all additional use cases I can see. But I would gladly validate if it works with your svg clock if you want.

jorgebucaran · 2017-03-22T10:47:34Z

@ngryman Cool, that would be this one http://codepen.io/jbucaran/pen/PWMBLp

But really, the problem has to do with what I wrote on Slack.

This may happen after keyed dom, so maybe we can close this PR and create a new one.

I don't know if it's my browser or GitHub, but it takes a while to load this thread every time I come here. How's your experience?

ngryman · 2017-03-22T11:10:05Z

Here is the working version with my PR applied: http://codepen.io/ngryman/pen/mWxoRo.

ngryman · 2017-03-22T11:10:34Z

This may happen after keyed dom, so maybe we can close this PR and create a new one.

Or merge it? 😂

jorgebucaran · 2017-03-22T11:17:57Z

@ngryman The code works, but it breaks when you try to edit the HTML on the dev console, which is how I am testing an imaginary browser extension that can modify document.body.

The svg was where I first found the problem, but even the simplest example can repro the issue.

ngryman · 2017-03-22T11:23:48Z

@jbucaran Can you reproduce it with programmatic calls? I'm not sure how devtools handle live edits exactly.
IMHO this is a super edge case. If we consider the current behavior (which breaks in many cases) and the good things that this PR brings, we should merge it, and then address your specific issue. I have the feeling we want to resolve too many things at the same time. What do you think?

jorgebucaran · 2017-03-22T13:45:17Z

Thank you @ngryman! 🤘 🤘

rbiggs · 2017-03-22T14:05:11Z

Thanx a ton @ngryman. I was playing with your pull request with the svg clock. I noticed I could insert the clock into this structure and update the p without affecting the clock :-)

<h1 id='testTitle'>
    <p>Hello</p>
</h1>

However, inserting the clock into a tag with only text and trying to update the textContent replaced the clock. Which is what I would expect:

<h1 id='testTitle'>Hello</h1>

👍

zaceno · 2017-03-22T15:25:19Z

Good work @ngryman (and @jbucaran) -- great to see this finally merged! (...now I gotta go fix the conflict in my PR though... ;) )

jorgebucaran mentioned this pull request Mar 7, 2017

Children appended to script element when using root: document.body #128

Closed

Support roots with existing children (jorgebucaran#128)

1010885

ngryman force-pushed the master branch from 72fdd89 to 1010885 Compare March 22, 2017 09:34

jorgebucaran merged commit 6f9d4c8 into jorgebucaran:master Mar 22, 2017

jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018

Support non-empty root. Fix #128. Fix #140.

5daa07b

Support roots with existing children (#128) #140

Support roots with existing children (#128) #140

Conversation

ngryman commented Mar 7, 2017

FlorianWendelborn commented Mar 7, 2017

codecov bot commented Mar 7, 2017 • edited

Codecov Report

ngryman commented Mar 7, 2017 • edited

ngryman commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

FlorianWendelborn commented Mar 7, 2017

leeoniya commented Mar 7, 2017 • edited

jorgebucaran commented Mar 7, 2017 • edited

FlorianWendelborn commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

FlorianWendelborn commented Mar 7, 2017 • edited

ngryman commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

FlorianWendelborn commented Mar 7, 2017 • edited

ngryman commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

ngryman commented Mar 7, 2017

FlorianWendelborn commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

ngryman commented Mar 7, 2017

ngryman commented Mar 7, 2017

ngryman commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

ngryman commented Mar 7, 2017

farism commented Mar 7, 2017

jorgebucaran commented Mar 7, 2017

leeoniya commented Mar 7, 2017 • edited

farism commented Mar 7, 2017

ngryman commented Mar 8, 2017 • edited by jorgebucaran

lukejacksonn commented Mar 15, 2017 • edited

jorgebucaran commented Mar 15, 2017 • edited

ngryman commented Mar 15, 2017

jorgebucaran commented Mar 15, 2017 • edited

ngryman commented Mar 15, 2017

jorgebucaran commented Mar 15, 2017 • edited

FlorianWendelborn commented Mar 15, 2017

jorgebucaran commented Mar 15, 2017 • edited

jorgebucaran commented Mar 15, 2017 • edited

jorgebucaran commented Mar 16, 2017 • edited

ngryman commented Mar 16, 2017

jorgebucaran commented Mar 16, 2017

zaceno commented Mar 21, 2017

jorgebucaran commented Mar 22, 2017

ngryman commented Mar 22, 2017

ngryman commented Mar 22, 2017

jorgebucaran commented Mar 22, 2017

ngryman commented Mar 22, 2017 • edited

jorgebucaran commented Mar 22, 2017 • edited

ngryman commented Mar 22, 2017 • edited

ngryman commented Mar 22, 2017 • edited

jorgebucaran commented Mar 22, 2017

ngryman commented Mar 22, 2017 • edited

jorgebucaran commented Mar 22, 2017

rbiggs commented Mar 22, 2017

zaceno commented Mar 22, 2017

codecov bot commented Mar 7, 2017 •

edited

ngryman commented Mar 7, 2017 •

edited

leeoniya commented Mar 7, 2017 •

edited

jorgebucaran commented Mar 7, 2017 •

edited

FlorianWendelborn commented Mar 7, 2017 •

edited

FlorianWendelborn commented Mar 7, 2017 •

edited

leeoniya commented Mar 7, 2017 •

edited

ngryman commented Mar 8, 2017 •

edited by jorgebucaran

lukejacksonn commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 15, 2017 •

edited

jorgebucaran commented Mar 16, 2017 •

edited

ngryman commented Mar 22, 2017 •

edited

jorgebucaran commented Mar 22, 2017 •

edited

ngryman commented Mar 22, 2017 •

edited

ngryman commented Mar 22, 2017 •

edited

ngryman commented Mar 22, 2017 •

edited