Clarification on nested templates, conditionals and side effects

[michaelsbradleyjr]

With reference to sample code:

http://jsbin.com/aqaFOCa/1/edit

https://gist.github.com/michaelsbradleyjr/6400707

The template which repeats over multiples is nested inside a template which is predicated on the value of base being an even number.

However, in cases where base is not even, though the nested template is not rendered (the template for the non-even case is rendered instead), the t1Changed method for the test-nested-li elements is still being called, as evidenced in the "Log" section at the bottom of the page.

This might be seen as a surprising result inasmuch as wrapping JavaScript code in an if will prevent said code from causing side effects when the expression inside if (...) evaluates to something falsy.

However, given the nature of MDV bindings, perhaps this kind of behavior is expected and needs to be planned for when writing methods like t1Changed.

Clarification would be appreciated.

[michaelsbradleyjr]

I made a few changes and updated the example to load yesterday's 2013-09-05 release:

http://jsbin.com/IdacEjO/4/edit

For comparison, here is the revised code with the 2013-08-16 release:

http://jsbin.com/EyePOLU/1/edit

The example behaves differently with the 09-05 release than it does with the 08-16 release — it seems "more broken" with the latest release, though both exhibit behavior that seems (to me) surprising, per the criteria in my original description for this issue.

Granted, this example is a bit contrived; that aside, there may be some conceptual mistakes in my code and any insights would be appreciated.

[dfreedm]

Ok, so the problem here is that {{base}} is tightly bound to {{t1}} in <li is="test-nested-li">.

When {{base}} changes, {{t1}} instantly changes, then the <template if> surrounding it cleans up the node.

This process has been a topic for discussion recently, and had only theorized on this problem's existence. Thank you for posting it.

[michaelsbradleyjr]

@azakus thank you for taking a look and for the explanation. I'm glad to be of assistance.

[sjmiles]

Fwiw, while it's true that base and t1 are tightly bound, the actual issue here is the timing of the observer callbacks.

In other words, base and t1 have both changed, and the side-effects of those changes are always asynchronous.

As it happens in your example, the t1 changed callback (output to the log) happens before the base changed callback (causing the template to re-render).

In general, we want you to design your application so it's immune to the order of asynchronous change observers, but please follow up if this is a problem.

It's possible we can rearrange the order of callbacks to make templates 'go first' in the context of a single element, but it's not a panacea.

Scott

On Tue, Sep 10, 2013 at 6:04 PM, Michael Bradley notifications@github.comwrote:

@azakus https://github.com/azakus thank you for taking a look and for the explanation. I'm glad to be of assistance.

— Reply to this email directly or view it on GitHubhttps://github.com/Polymer/polymer/issues/265#issuecomment-24206479 .

[michaelsbradleyjr]

@sjmiles thank you for the additional clarification.

I have made a point of late to be wary of asynchronicity and non-determinism when composing polymer-elements.

I think, though, this will be a ready pitfall for many developers. That is, many will likely reason that a template if can be used in an analogous fashion to a JavaScript if, and it may incidentally work out that way in many cases, reinforcing the idea.

So perhaps some "heads up" note/s on polymer-project.org might be called for.

[johnmccutchan]

Hi,

I have the following structure:

<template>
  <template if="{{isPrimitive}}">
     <!-- More {{ }} expressions which assert isPrimitive is true }} -->
  </template>
  <template if="{{isList}}">
     <template repeat="{{ item in list}}">
       <!-- More {{ }} expressions which assert isList is true }} -->
     </template>
  </template>
  <template if="{{isMap}}">
     <template repeat="{{key in map.keys}}">
       <!-- More {{ }} expressions which assert isMap is true }} -->
     </template>
  </template>
</template>

At any given time only one of isPrimitive, isList, and isMap can be true. What I'm seeing is that the {{ }} expressions contained in the two repeat templates are always evaluated despite the fact that there nesting template conditional is false. The page only displays the correct one. This is confusing to the point of being troubling.

[sjmiles]

The original issue here was specifically due to a complicated series of interactions with code. If you believe this is the same problem you are having, we will need to see your JavaScript.

If you are not sure, it would be best to repost this question in it's own issue.

Either way, please define what you mean by 'seeing that the {{}} expressions contained ... are ... evaluated'. Do you refer to bad rendering output, or something you can observe while debugging?

[I fixed your post by adding the GitHub triple-backtick syntax hinting]

[sethladd]

/sub

[johnmccutchan]

Note: I'm using Polymer.dart. The only Dart code is:

bindProperty(this, const Symbol('json'), () { notifyProperty(this, const Symbol('isMap')); notifyProperty(this, const Symbol('isList')); notifyProperty(this, const Symbol('isPrimitive')); });

I believe my issue is the same as the threads. I know the {{ }} expressions are being evaluated because my log is spammed with exceptions. The page displays correctly but code that I don't expect to execute does execute.

[sjmiles]

What kind of exceptions? I suspect what you have here is a problem specific to the Dart port.

[johnmccutchan]

I think the issue is again that property notifications are delivered in an order my templates were not expecting. The issue seems to be that when "json" changes the templates are evaluated and json isn't the type they were expecting. As a side effect the isMap, isList, and isPrimitive properties are all updated and the page renders correctly.

I've managed to fix my element by removing all references to "json" within {{ }}. When json is updated I explicitly notify the properties (isList, isPrimitive, isMap), etc and then use special functions to access the data I would get from within json.

[sjmiles]

I'm concerned that your diagnosis is improper, but I cannot evaluate it because I don't have enough information. For example, I don't know off hand what the JS analog is to the Dart code you showed.

[johnmccutchan]

Complete element code:

<!DOCTYPE html>

<html>
  <body>
    <polymer-element name="json-view" attributes="json">
      <template>
        <div>
        <template bind if="{{ valueType == 'Primitive' }}">
          <p>{{primitiveString}}</p>
        </template>
        <template bind if="{{ valueType == 'List' }}">
          <table class="table table-bordered table-hover">
            <caption class="text-left">List, {{list.length}}</caption>
            <tbody>
              <tr template repeat="{{item in list)}}">
                <th>{{counter}}</th>
                <td><json-view json="{{item}}"></json-view></td>
              </tr>
            </tbody>
          </table>
        </template>
        <template if="{{ valueType == 'Map' }}">
          <table class="table table-bordered table-hover">
            <caption class="text-left">Object, {{keys.length}}</caption>
            <tbody>
              <tr template repeat="{{key in keys}}">
                <th>{{key}}</th>
                <td>
                  <json-view json="{{value(key)}}"></json-view>
                </td>
              </tr>
            </tbody>
          </table>
        </template>
        </div>
      </template>
      <script type="application/dart" src="json_view.dart"></script>
    </polymer-element>
  </body>
</html>

library json_view_element;

import 'package:polymer/polymer.dart';

@CustomTag('json-view')
class JsonViewElement extends PolymerElement with ObservableMixin {
  @observable
  var json = 'foo';
  var count = 0;

  JsonViewElement() : super() {
    bindProperty(this, const Symbol('json'), () {
      count = 0;
      notifyProperty(this, const Symbol('valueType'));
    });
  }

  void created() {
   super.created();
 }

  void inserted() {
    super.inserted();
  }

  void removed() {
    super.removed();
  }

  String get primitiveString {
    return json.toString();
  }

  String get valueType {
    if (json is Map) {
      return 'Map';
    } else if (json is List) {
      return 'List';
    }
    return 'Primitive';
  }

  int get counter {
    return count++;
  }

  List get list {
    if (json is List) {
      return json;
    }
    return [];
  }

  List get keys {
    if (json is Map) {
      return json.keys.toList();
    }
    return [];
  }

  dynamic value(String key) {
    return json[key];
  }
}

[sjmiles]

Aside: please review this information about posting code: https://help.github.com/articles/github-flavored-markdown#fenced-code-blocks

[johnmccutchan]

Note, the above code is the working version.

[sjmiles]

I'm not sure why you posted the working version. The idea here is to diagnose the failure condition. Please post the problem code.

The original poster's problem was very specific as I noted, I still do not see indications that you are triggering the same problem.

Additionally, you are posting code that uses a port of this library. You are not using our code, so even with the proper failing test we may not be able to clear this up.

If it seems like I'm making a big deal out of this, it's because there is danger of FUD around this issue, and I want to maintain as much clarity as possible.

[johnmccutchan]

I'm not trying to spread FUD. I'm under a deadline to get a working system (which is why I only had working code...). Anyways, I've gone ahead and backed out my changes. Here is a version of the .html which exhibits the errors:

<!DOCTYPE html>
<html>
  <body>
    <polymer-element name="json-view" attributes="json">
      <template>
        <div>
        <template bind if="{{ valueType == 'Primitive' }}">
          <p>{{primitiveString}}</p>
        </template>
        <template bind if="{{ valueType == 'List' }}">
          <table class="table table-bordered table-hover">
            <caption class="text-left">List, {{list.length}}</caption>
            <tbody>
              <tr template repeat="{{item in json)}}">
                <th>{{ item }}</th>
                <td><json-view json="{{item}}"></json-view></td>
              </tr>
            </tbody>
          </table>
        </template>
        <template if="{{ valueType == 'Map' }}">
          <table class="table table-bordered table-hover">
            <caption class="text-left">Object, {{json.keys.length}}</caption>
            <tbody>
              <tr template repeat="{{key in keys}}">
                <th>{{key}}</th>
                <td><json-view json="{{json[key]}}"></json-view></td>
              </tr>
            </tbody>
          </table>
        </template>
        </div>
      </template>
      <script type="application/dart" src="json_view.dart"></script>
    </polymer-element>
  </body>
</html>

The errors are all repetitions of this pattern:

Uncaught Error: Class 'List' has no instance getter 'keys'.

NoSuchMethodError : method not found: 'keys'
Receiver: GrowableObjectArray len:4
Arguments: []

My analysis is as follows:

1. json is changed by code.
2. This triggers the inner template which uses json within {{ }} to be re-evaluated resulting in my runtime exceptions.
3. The following code is executed, causing templates which uses valueType to be re-evaluated.

bindProperty(this, const Symbol('json'), () {
      count = 0;
      notifyProperty(this, const Symbol('valueType'));
    });

Which results in the correct outer template being picked and the page rendering correctly.

[sjmiles]

I know you are not trying to spread FUD, but FUD comes along with confusing or partial information in issues.

Thank you for the html, but now you have omitted your logic, which as I have said, is critically important. However, at this point, I believe you must take this to the Polymer-Dart team.

The Polymer code has specific implementations to ensure that property observers occur before template observers (at least for the first pass). The non-Dart version is also robust against nulls (at least, with respect to throwing exceptions).

If there really is a design problem here, I suggest the Polymer-Dart guys will be able to help isolate it more clearly.

[johnmccutchan]

The logic is exactly the same as in the working case (so see above for the .dart code).

[sjmiles]

bind|notifyProperty is Dart specific, so we cannot speak to timing issues there.

[johnmccutchan]

Cool, I've notified the Dart porters of this issue and perhaps there is an order issue in the Dart port.

Thanks for the fast responses.

[jmesserly]

@sjmiles -- sorry I did not see this discussion before now. I will take a look. Definitely feel free to summon me via @jmesserly if you ever need :)

[sjmiles]

@jmesserly: No worries, this is an important issue and I know the Dart port is high quality, so it's worth it to work the problem together.

@johnmccutchan: thanks for hanging with me, I want to make sure we get to the bottom of this problem for everybody's sake.

[jmesserly]

Thank you Scott. That is perhaps a bit too charitable :). We are indeed trying to reach a very high quality port, across every level of the Polymer's arch diagram, but we're still missing some pieces. In particular:

1. The Polymer core -- this github project -- is only partially ported. Properties in particular are not in a good state. I am working on a change now that will port it all to the highest fidelity possible. (Some things involving prototypes don't translate directly; I am trying to find the closest analog in a class based language. I would love feedback on how the APIs translate, by the way. Maybe we can schedule something?)
2. It is not in the arch diagram, but: https://github.com/Polymer/observe-js, which came from Rafael's ChangeSummary library. Back then it seemed like it wasn't a public API of the Polymer stack, but it is in a much better place now with some really nice building blocks for observation.

In any event -- bind and notify come from our observe package, roughly equivalent to observe-js, but it predates it and I have not had a chance to reconcile APIs (some like PathObserver are decently similar). bindProperty is shorthand for a PathObserver. notifyProperty is rougly shorthand for http://wiki.ecmascript.org/doku.php?id=harmony:observe#notifierprototype .

So the JS equivalent to John's code is roughly:

new PathObserver(this, 'json', function(x) {
  function notifyProperty(name) {
    Object.getNotifier(this).notifty({
          object: this,
          type: 'update',
          name: name
        });
  }
  notifyProperty('isMap');
  notifyProperty('isList');
  notifyProperty('isPrimitive');
}, this);

I'd be interested trying a similar example in polymer.js. IIRC, PathObservers should deliver changes in birth order, and ones inside a <template if> will be created after that one, so the callback above should run before the templates try to evaluate keys. However, by the time they notify the change to isMap we might still be processing the change to json. So using pure TemplateBinding, this bug seems possible to trigger.

I'm curious how Polymer avoids this. Is it because of the *Changed handlers, which are smarter than a pure PathObserver?

[sjmiles]

Unfortunately the nature of the observer implementation is critically important here. There are a few different implementations, and none of them have precisely the same behavior.

The exact problem brought up here wouldn't be noticed in Polymer for a couple of reasons: (1) it's unlikely a user is actually using Object.observe and the polyfill has different characteristics and (2) we are more laissez-faire than Dart wrt to types and nulls.

IOW, I think there are actual issues here, it's just not clear exactly what recipe to optimize.

One proposal is the notion of a late-acting observer. Most of these problems have to do with templates updating before the notifier stack is emptied. If we could have certain observers that don't process until the model has stabilized, a deferred-observer if you will, maybe we could avoid the issue by deferring template observers.

[jmesserly]

If we could have certain observers that don't process until the model has stabilized, a deferred-observer if you will, maybe we could avoid the issue by deferring template observers.

yeah, that does seem like a nice solution. Thinking of spreadsheet/FRP models, it is nice to have the "model computation" happen completely before the UI responds, so it sees a consistent state. I seem to recall @blois had a similar suggestion.

(2) we are more laissez-faire than Dart wrt to types and nulls.

yeah, to the extent it is in the Polymer Expressions or TemplateBindings, it should be just as null-tolerant. However, we've occasionally had bugs in our Polymer Expressions implementation. As far as code written in both JS or Dart, "obj.foo" is an error if obj is null, right?

[sjmiles]

"obj.foo" is an error if obj is null, right?

Absolutely.

Otoh, if obj is not null, obj.keys will never return

Uncaught Error: Class 'List' has no instance getter 'keys'.

:)

[jmesserly]

that's fair, it would return undefined in JS. But if you try to do anything with undefined, you often get something like:

> [].keys.keys
TypeError: Cannot read property 'keys' of undefined

...so it's kind of a shallow behavior. Always surprises me a bit, because usually if you're going to go with dataflow error propagation in a language, you make it so that operations applied to the error object result in the same error.

Anyways :), in this case I'm fairly sure it's a bug in the Dart version of Polymer Expressions: https://code.google.com/p/dart/issues/detail?id=13393

(our TemplateBinding layer would not issue an error for getting "keys" from a list.)

[jmesserly]

by the way -- I am porting Polymer code related to properties, and I think the bind/notifyProperty pattern John used in Dart is semantically identical to Polymer's bindProperty. It boils down to the same primitives from the perspective of observe.js. So if the Dart version fails but the Polymer.js version works, it means we have a bug in Dart's observe package.

@sjmiles -- what did you mean by the difference between polyfilled and native Object.observe? Shouldn't they have the same delivery order? As far as I can tell from the observe.js code, Rafael was very careful to make the polyfill behave the same as native wherever possible (the biggest difference that I know: the need for manual cleanup in the polyfilled case).

[rafaelw]

This is a pretty subtle problem. In some sense, the problem is an invalid assumption:

-The template syntax looks imperative (ifs, repeats, etc...), so it should behave like imperative code.

This would only be true if the behavior of the templating system was to destroy itself entirely when any dependent data changed, and re-render progressively.

Obviously, doing that would come at a huge cost: terrible performance and loss of DOM stability (don't destroy elements if you don't have to -- they might have important transient state like uncommitted forms values or event listeners).

I'm very wary of trying to solve this problem by asserting some kind if prioritization of observers. I think no matter what prioritization we pick, you can construct an example (WRT the above assumption) that will be fail. Case in point, the two examples on this bug both have at their root observing the state of same value "too soon", but one does the observation within the template while the other does the observing within its element (thus asserting that one or the other go first, doesn't solve the problem generally).

For better or worse, the system we have is asynchronous with chained observations. When data changes, it will propagate through the system and reach observers at potentially different times based on how "far" each observer is from the root value that changed.

Obviously, we'd like a system with as few rough edges as possible. I'll definitely think more about this. However, I think that trying to maintain invariants via the ordering of observer callbacks is a loosing proposition. E.g. I'm going to assume that I'll never see a "base" value which is odd because some other observer will have destroyed me before I can --- is probably just unworkable.

[michaelsbradleyjr]

@rafaelw thanks for the feedback. I do appreciate that this is a complex issue and that it may take some time for an optimal arrangement to get worked out.

For what it's worth, Flapjax deals with with this issue by way of a topologically sorted propagation graph. However, the implementation also depends on synchronous propagation (i.e. synchronous through the graph of EvenStreams and Behaviors). I've had some ideas around how such a topological ordering could be leveraged in the context of async propagation, but it's been awhile since I entertained those thoughts. By the way, I realize that observe-js and Polymer's templates don't share a common implementation with Flapjax, but there is some conceptual cross-over and I thought you might find it interesting.

[rafaelw]

Yeah, I'm aware that Flapjax (Elm, etc...) have a "supervisor" approach to this. That may ultimately be the "right" solution to this problem, but it introduces another problem: needing to know all the nodes in the system. One goal of our system is that it compose cleanly with other mechanisms. IOW, it's safe to throw elements together that weren't designed with knowledge of each other. Requiring that all observers in the system register with some kind of scheduler goes against that compose-ability.

The other problem is garbage collection. We don't have WeakRefs in JS, but when we do, observers should be collectable when their clients are no longer reachable and we can do away with the "dispose" pattern, which is brittle. Again, registering with "the system" goes against this.

Obviously, we can't have it all. =-(. This may be where the opinionated part of polymer comes into play. Perhaps if Flapjax or Elm decides to make a web components framework, they will make different trade-offs.

BTW, I'm not at all disagreeing with you. Just lamenting that we can't have it all.

[michaelsbradleyjr]

Agreed regarding the global scheduler being at odds with the "small, independently composable units" approach, the latter being what attracted me to Polymer right off the bat.

An older version of Flapjax did have a mechanism for dealing with garbage collection, but the current maintainer removed it from the codebase back in 2011 (development of the codebase stretches back to 2006, amazingly, though it was moved to GitHub in 2010). As a learning experience, I once tried rewriting Flapjax from scratch in CoffeeScript, and I remember at the time finding some bugs in the original "gc" code which prevented it from doing anything useful in most cases, which is probably why it got removed. I fixed the bug (i.e. in my re-impl) and had working tests around that, but I stopped working on it in early 2012, so I don't recall all the details offhand. Needless to say, the cleanup code was heavily interwoven with the global scheduler, but I remember being impressed that it worked in a surprisingly elegant way -- backtracking from and unlinking dead nodes (flagged as such) in the graph, thus freeing them up for gc.

I saw Rich Hickey present on Clojure's core.async last week, and it struck me as being a really nice mechanism for decoupling async coordination from business logic -- the API is inspired by Hoare's CSP formalisms. It's built up in terms of macros, though, so unfortunately it's not possible to simply "export" the API to vanilla JS in the form of pre-compiled ClojureScript, a la mori. Even if one could do that, I don't know yet how it could be used as a basis for a "better" system of declarative data bindings, though I've been thinking about the problem.

[tjsavage]

Closing this issue due to age and the release of version 1 release of Polymer - please feel free to re-open if this is incorrect.