search

googlearchive / TemplateBinding

TemplateBinding Prolyfill
290 stars 61 forks source link

Unbinding a model does not completely make the template's contents disappear #128

Closed jmesserly closed 11 years ago

jmesserly commented 11 years ago

(original bug https://code.google.com/p/dart/issues/detail?id=11983)

I took @sethladd 's example and integrated it into MDV's sample.html:

<!DOCTYPE html>
<html>
  <head>
    <script src="mdv.js"></script>
  </head>
  <body>
    <p>
      The below will appear and disappear as a model object is
      bound and unbound from the template. Conditionals are implemented
      simply as "this template appears if something is bound" and
      "this template disappears if nothing is bound".
    </p>

    <template id="greeting" bind>
      <p>Hello {{msg}}</p>
    </template>
  <script>
  document.addEventListener('DOMContentLoaded', function() {
    var t = document.getElementById('greeting');
    var model = { msg: 'world' };
    t.model = model;

    // Needed to detect model changes if Object.observe
    // is not available in the JS VM.
    Platform.performMicrotaskCheckpoint();

    setInterval(function() {
      if (!t.model) {
        t.model = model;
      } else {
        t.model = void 0;
      }
      Platform.performMicrotaskCheckpoint();
    }, 1000);

  });
  </script>
  <body>
</html>
jmesserly commented 11 years ago

btw, this has the same results with either null or undefined

sethladd commented 11 years ago

Thanks for filing.

jmesserly commented 11 years ago

hmm, thinking about this some more, I think you need to add an if to the template. However, when I do that, it still expands the template, which is not what I'd expect. Now I'm confused :)

I did get it to work by adding if="{{}}" ... this makes me wonder if if should work like bind and repeat, where an empty attribute implies '{{}}' ?

Here is the working version:

<!DOCTYPE html>
<html>
  <head>
    <script src="mdv.js"></script>
  </head>
  <body>
    <p>
      The below will appear and disappear as a model object is
      bound and unbound from the template. Conditionals are implemented
      simply as "this template appears if something is bound" and
      "this template disappears if nothing is bound".
    </p>

    <template id="greeting" if='{{}}'>
      <p>Hello {{msg}}</p>
    </template>
  <script>
  document.addEventListener('DOMContentLoaded', function() {
    var t = document.getElementById('greeting');
    var model = { msg: 'world' };
    t.model = model;

    // Needed to detect model changes if Object.observe
    // is not available in the JS VM.
    Platform.performMicrotaskCheckpoint();

    setInterval(function() {
      if (!t.model) {
        t.model = model;
      } else {
        t.model = void 0;
      }
      Platform.performMicrotaskCheckpoint();
    }, 1000);

  });
  </script>
  </body>
</html>
sethladd commented 11 years ago

Thanks for researching this, John. I asked more about what bind means, and if this is really a bug, on this thread: https://groups.google.com/forum/?fromgroups=#!topic/polymer-dev/nBdPaVMtcys

if="{{}}" and just if seem redundant if bind means "this template should activate and insert into the document if a model is bound". There's already an implicit if in there. But I might not understand the semantics of bind.

jmesserly commented 11 years ago

There's already an implicit if in there. But I might not understand the semantics of bind.

That's just the thing, there isn't an implicit "if". "If" implies "bind", not the other way around. "bind" just means "make this template active using the provided model". You can also use it to get at a field, e.g. <template bind="{{foo.bar.baz}}">the value of foo.bar.baz.qux is: {{qux}}</template>

jmesserly commented 11 years ago

You can think of bind as a 1-element repeat. (Which is exactly how it's implemented)

sethladd commented 11 years ago

Thanks John. This is clear to me: "make this template active using the provided model". However, shouldn't the opposite be true? ""make this template inactive if there is no model" That is how it works when the page initially starts up, before I ever bind to that template.

Maybe.... I'm "unbinding" by setting model to null (in my Dart version). Perhaps null isn't enough to fully and truly unbind?

jmesserly commented 11 years ago

right, I don't think there is a way to "unbind" a template, unless it has the "if" attribute. "null" is generally a valid model value.

It is rather strange that this has a side effect which cannot be reversed:

var node = querySelector('template'); // note: template has "bind" attribute
node.model = node.model; // set property to itself--harmless right?

you can reverse it in a few different ways:

node.unbindAll(); // option 1
node.unbind('bind'); // option 2
node.bind('if', false, ''); // option 3

None of those are very satisfying. Also I think MDV will leak a little bit of memory even with unbindAll (IIRC, the TemplateIterator internal object isn't freed when all of the TemplateBinding objects are unregistered).

sethladd commented 11 years ago

So that begs the question, how can you ask if a template has something that is bound? I was doing template.model == null but I guess that's a valid binding.

jmesserly commented 11 years ago

Hmm, it should be possible to do via template.bindings.length == 0. I'm not sure if that is intended or not, but there is now a "bindings" Map exposed on Node (see Dart's Node.bindings).

In JavaScript it's probably safer to do if (template.bindings) since it can be undefined as well.

rafaelw commented 11 years ago

So, I agree with you guys that the current semantics are a bit weird. I'm at a loss as to what the "right" behavior is.

One option is that template instances will never be created if the model is undefined.

That would have the effect that <template bind>, template.model = undefined will always result in zero instances.

However, to be consistent with that, I think the needs to apply to repeat, so

<template repeat>, template.model = new Array(10)

would also produce zero instances (which pretty much everyone agrees is wrong).

This brings me to the fact that template instances must tolerate any model value.

FWIW, the real way to remove a "bound" instance is to unbind it, e.g.

template.unbind('bind').

That will remove the instance regardless (always).

template.model =

is really just a short hand for

1) look at the attributes of the template, 2) call bind() on each with the model value.

If you guys can think of a better way to express this API, I'm all ears.

sethladd commented 11 years ago

Thanks for the comments. I'm not sold that template instances must tolerate undefined or null as acceptable model values.

If template.model = foo calls bind() on each attribute of the template, then I would expect template.model = null to call unbind() on each attribute of the template.

It's weird to let me call .model = foo but then to undo that I need to call .unbind('bind'). It's not analogous.

sethladd commented 11 years ago

Raf, can you confirm that "FWIW, the real way to remove a "bound" instance is to unbind it, e.g." also implies that the template is removed from the DOM?

rafaelw commented 11 years ago

Seth: Yes.

rafaelw commented 11 years ago

Ok. So here's my proposal: add a clear() method to template which has the effect of removing all instance (by way of calling unbind('bind'), unbind('if') and unbind('repeat') on itself.

jmesserly commented 11 years ago

Sounds reasonable to me.

rafaelw commented 11 years ago

clear() implemented here: https://github.com/Polymer/mdv/commit/60d98f19c4137c4d15821171171d67468398758d