prepopulate node cache - Githubissues

sanderhahn commented 10 years ago

In React it is possible to reuse serverside generated html/dom nodes. This is useful when the server already generates a page representation for seo purposes. Is it possible to do something simular in Mithril?

Workaround at this moment seems to explicitly remove any serverside generated html. This seems kind of hacky, is there a nicer way?

<!doctype html>
<body>
<div id="test">
  <p>Server Test</p>
</div>
<script src="/js/mithril.js"></script>
<script>
var app = {
  controller: function() {
    this.paragraph = 'Client Test'
  },
  view: function(ctrl) {
    return m('p', ctrl.paragraph)
  }
}

function cleanUp(node) {
  while (node.firstChild) {
    node.removeChild(node.firstChild)
  }
}

var mountAt = document.getElementById('test')
cleanUp(mountAt)
m.module(mountAt, app)
</script>
</body>

Raynos commented 10 years ago

:+1:

I'm very interested in learning how to properly render a virtual DOM onto a pre-rendered HTML version.

sanderhahn commented 10 years ago

Just fiddling around a bit in the build method and trying to lookup elements in the same parent by their tagname. When tags are not found just keep the default create logic. Mark elements that are used or created and remove elements that are not. Not sure if it is much faster and if it will support all conditions... Will let you know if i have something in somewhat usable state :-)

lhorie commented 10 years ago

Currently, this isn't supported. The main problem with making this possible is that if an AOP-style library like Bootstrap runs before Mithril, Mithril could be trying to reuse elements that have events attached (and possibly double-attaching the same events via configs and whatnot if the server-side generation stuff is done as an afterthought after code complete).

Another possible workaround for the SEO case specifically would be wrapping the pre-generated markup in a <noscript> tag, but of course, this does nothing to improve performance.

Raynos commented 10 years ago

@lhorie how could the double attaching work ?

the html that the page loads should be just html. I think were are talking about m.module(container, app) where container is not empty but instead contains server side generated HTML.

Why do you need a BootStrap. What could cause the double attaching ?

lhorie commented 10 years ago

For example, let's say you have a legacy app:

<div>some legacy stuff</div>

<!--mithril widget gets added here-->
<div id="foo"></div>
<!--end mithril widget-->

<div>
  some more legacy stuff
  <div class="modal">...</div>
</div>
<script>$(".modal").modal()</script>

<script>
//new mithril code
function modal(el) {
  $(el).modal()
}
m.render(document.getElementById("foo"), m(".modal", {config: modal}, ["etc"]))
</script>

With the pre-generated markup:

<div>some legacy stuff</div>

<!--mithril widget gets added here-->
<div id="foo">
  <div class="modal">etc</div>
</div>
<!--end mithril widget-->

<div>
  some more legacy stuff
  <div class="modal">...</div>
</div>
<script>$(".modal").modal()</script>

<script>
//new mithril code
function modal(el) {
  $(el).modal()
}
m.render(document.getElementById("foo"), m(".modal", {config: modal}, ["etc"]))
</script>

Notice that we're now calling .modal() on the div inside #foo twice, so if .modal internally does .addEventListener("click", toggleSomething) or whatever, then we end up with two identical event handlers and clicking would toggle twice.

Raynos commented 10 years ago

@lhorie that's a bug with server side generated HTML full stop.

It's not a bug with m.render().

This is a bug of server side code, not client side code.

Even without the // new mithril code <script> this would break.

I don't think this is something you can gaurd against. It's a user error, not a mithril error. The correct solution is that the legacy stuff should do $('.legacy-container .modal'). It's a classic global scope bug that can only be fixed by writing not global code.

lhorie commented 10 years ago

It's not really a matter of who causes the bug. Like I mentioned before, some AOP-style libraries activate on inclusion, and you don't always get an API to tell them what to leave intact. Or maybe you do, but the library is used so pervasively that the cost of refactoring is prohibitive, etc.

The bottom line, though, is that trying to sneak in an optimization can cause semantic surprises that could be blamed on Mithril not playing well with others, whereas doing something similar to @sanderhahn's workaround (i.e. clearing the pre-gen'ed DOM and re-building from scratch) always does what people expect. As the saying goes, if it doesn't need to be correct, it's easy to optimize it to cost zero time.

Also mind you, while this is my main concern wrt to this feature, it's not the only one:

supporting pre-gen'ed DOMs breaks Mithril's immunity to FOUC,
the performance cost of scanning through every attribute in every element to build the cache worries me due to Element's monstrously large signature,
I don't know if you actually get any performance benefits from downloading markup in addition to the Mithril-equivalent template, as opposed to, say, a user-agent based output switch for the SEO case (Remember dynamic markup invalidates HTML caching, etc)
etc

Anyways. I'm not trying to rain on the parade. I'd like for this feature to exist as much as the next guy, but I want to make sure to have a good understanding of the pros and cons first.

Raynos commented 10 years ago

@lhorie first page load of js takes n seconds in Africa HTML renders instantly and doesn't need a second http request for js.

sanderhahn commented 10 years ago

Ah noscript is nicer than removing the elements :-)

Wanted to limit my experiment for the SEO use case. I understand that solving the generic problem of making the virtual dom interoperate nicely with custom dom manipulation is much harder.

In this experiment the build method is customized to lookup reusable nodes (based on tagname), it removes attributes that are not managed by Mithril and does cleanup of any remaining unmanaged nodes.

https://github.com/sanderhahn/mithril.js/commit/973bbba7eb4505d464d20809d6961151670cd76c

Removing event handlers on nodes might be possible using this purge function: http://javascript.crockford.com/memory/leak.html However i am not sure how you would differentiate between Mithril and non-Mithril event handlers.

See it in action here: http://jsfiddle.net/bRJdQ/1/

Let me know what you think :-)

<!doctype html>
<body>
<div id="list">
  <ul>
    <li style="color: green">Server <b>one</b></li>
    <li>Server <span>two</span>...</li>
    <li>Server three</li>
  </ul>
  <button>Add</button><button>Rotate</button>
</div>
<script>
var app = {
  controller: function() {
    this.list = ['Client one', 'Client two', 'Client three']
    this.click = function() {
      this.list.push('item ' + (this.list.length+1))
    }
    this.rotate = function() {
      this.list.push(this.list.shift())
    }
  },
  view: function(ctrl) {
    return [m('ul', [
          ctrl.list.map(function(item, index) {
            return m('li', item)
          }),
        ]
      ),
      m('button', {onclick: ctrl.click.bind(ctrl)}, 'Add'),
      m('button', {onclick: ctrl.rotate.bind(ctrl)}, 'Rotate')
    ]
  }
}
m.module(document.getElementById('list'), app)
</script>
</body>

lhorie commented 10 years ago

@sanderhahn That's a nice proof of concept!

I was looking around and it looks like there is one hackish way of removing event handlers from elements (and it does so in children too, which works out great). I'm gonna have to investigate the memory leak issue mentioned there (I believe it has to do w/ IE6 prior to Windows Service Pack 2 or 3, but I'm not sure). Overall, this is looking promising though :)

lhorie commented 10 years ago

For the record, there was also a thread on the mailing list on this topic recently:

https://groups.google.com/d/msg/mithriljs/pxpj2MMIRYs/ZbbJfQF-lEQJ

There, @Satyam provides a working demo with the server-side using a modified mock.js to pre-render.

On a semi-related note, I added a change to the way rendering works on route changes (they now do a full render, instead of attempting a diff), which, among other things, affects this.

The code now handles existing markup gracefully (i.e. it doesn't render the app underneath the existing markup), but @Raynos comment ("If between the time it takes for the server HTML to get rendered and the javascript to get loaded a user selects an and starts typing, he will have typed before event listeners were added.") still needs to be addressed somehow.

Unfortunately the hack I mentioned above doesn't work for our purposes, since cloning the nodes and overwriting the existing tree will kill DOM state just the same as what the diff engine does.

So tl;dr: the code now handles pre-gen markup and rogue 3rd party libraries, but not the FOUC issue. I'll open another issue for that separately.

Satyam commented 10 years ago

Server-generated code might add the disabled attribute to input controls (input, textarea and select) and buttons that have events attached. Since the page might have controls intentionally disabled, a data-keep-disabled attribute might be added to those.

On reviving the client side code, since these nodes have to be visited anyway to attach the event listeners, the disabled attribute might be deleted (unless data-keep-disabled is also true).

Nodes that don't have events listeners attached can be left enabled since it won't make a difference.

MithrilJS / mithril.js

prepopulate node cache #60