fix #136, support watch exprs and two way bindings for component fields

fix #136, support watch exprs and two way bindings for component fields

Committed: https://github.com/dart-lang/dart-web-components/commit/e321f1c

[Jennifer Messerly, 2012-11-29 05:34:34]

I'm not sure if we'll get to #82 for M2, but it will change a bunch of render
tree baselines so I figured a separate CL was in order.

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.dart
File lib/src/analyzer.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.da...
lib/src/analyzer.dart:124: node.attributes.forEach((k, v) =>
visitAttribute(info, k, v));
needs to happen after we bind "extends"

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart#n...
lib/src/info.dart:449: // TODO(jmesserly): turn this on when issue #82 is fixed.
I realized if we have "input" two way bindings but "<x-my-input>" html it
wouldn't work, unless we fix #82.

[Siggi Cherem (dart-lang), 2012-11-29 18:06:57]

very very cool.

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.dart
File lib/src/analyzer.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.da...
lib/src/analyzer.dart:343: 'field-name="{{expr}}". Unlike data-value "expr" will
be watched and '
add comma?
Unlike data-value, "expr" ...

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.da...
lib/src/emitters.dart:185: "${info.identifier} = new autogenerated.Text('');");
I'm not sure why is this changing?

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.da...
lib/src/emitters.dart:931: // elements, or try to set a property if this is a
component.
maybe split the comment in two? since the next statement only refers to the
component case?

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart#n...
lib/src/info.dart:311: }
how about doing this recursively?

String get baseExtendsTag =>
    extendsComponent == null ? extendsTag : extendsComponent.baseExtendsTag;

[Jennifer Messerly, 2012-11-29 19:26:02]

I'll fix the other things after I get out of training :)

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.da...
lib/src/emitters.dart:185: "${info.identifier} = new autogenerated.Text('');");
On 2012/11/29 18:06:57, Siggi Cherem (dart-lang) wrote:
> I'm not sure why is this changing?

Yeah, this one was tricky to figure out.

What's going on here is: this fixed a bug where we evaluate bindings too soo.
Generally we can't evaluate bindings in "created" because they don't have all of
their state set up yet. All of the bindings have the invariant that they are
evaluated in the context of "inserted_autogenerated".

This is not well specified:
https://github.com/dart-lang/dart-web-components/issues/226

For now I think this is the simplest/best solution--make text fields work like
other bindings and be evaluated in "inserted". I think there may be valid
reasons to have bindings be initialized sooner, but it's a change we should make
consistently.

There was also a minor issue that it was being evaluated twice at startup (here
and watchAndInvoke in inserted).

The concrete case is "TodoItemComponent" -- in created_autogenerated, the "todo"
property is still null, so "todo.task" is a nullreferencee

[Siggi Cherem (dart-lang), 2012-11-29 19:31:50]

lgtm

Thanks for the clarification.

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.da...
lib/src/emitters.dart:185: "${info.identifier} = new autogenerated.Text('');");
On 2012/11/29 19:26:02, John Messerly wrote:
> On 2012/11/29 18:06:57, Siggi Cherem (dart-lang) wrote:
> > I'm not sure why is this changing?
> 
> Yeah, this one was tricky to figure out.
> 
> What's going on here is: this fixed a bug where we evaluate bindings too soo.
> Generally we can't evaluate bindings in "created" because they don't have all
of
> their state set up yet. All of the bindings have the invariant that they are
> evaluated in the context of "inserted_autogenerated".
> 
> This is not well specified:
> https://github.com/dart-lang/dart-web-components/issues/226
> 
> For now I think this is the simplest/best solution--make text fields work like
> other bindings and be evaluated in "inserted". I think there may be valid
> reasons to have bindings be initialized sooner, but it's a change we should
make
> consistently.
> 
> There was also a minor issue that it was being evaluated twice at startup
(here
> and watchAndInvoke in inserted).
> 
> The concrete case is "TodoItemComponent" -- in created_autogenerated, the
"todo"
> property is still null, so "todo.task" is a nullreferencee

I see what you mean. We definitely need to do things consistently. I am hoping
we can do this on created though.

I think we are hitting again the issue that we basically want one more event in
the lifecycle. 

Let's have a design discussion later on about what to do here.

For now, this change LGTM.

[Jennifer Messerly, 2012-11-30 03:21:41]

Thanks!

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.dart
File lib/src/analyzer.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/analyzer.da...
lib/src/analyzer.dart:343: 'field-name="{{expr}}". Unlike data-value "expr" will
be watched and '
On 2012/11/29 18:06:57, Siggi Cherem (dart-lang) wrote:
> add comma?
> Unlike data-value, "expr" ...

Done.

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/emitters.da...
lib/src/emitters.dart:931: // elements, or try to set a property if this is a
component.
On 2012/11/29 18:06:57, Siggi Cherem (dart-lang) wrote:
> maybe split the comment in two? since the next statement only refers to the
> component case?

Done. Reworded to only refer to the "xtag.$name" case

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/11416259/diff/5001/lib/src/info.dart#n...
lib/src/info.dart:311: }
On 2012/11/29 18:06:57, Siggi Cherem (dart-lang) wrote:
> how about doing this recursively?
> 
> String get baseExtendsTag =>
>     extendsComponent == null ? extendsTag : extendsComponent.baseExtendsTag;

Done.