feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when #1001

mvuksano · 2014-05-05T14:16:34Z

setting up observing.

In case an element does not have an attribute set for a property defined
in NodeAttrs that property is not automatically initialised. Properties
that are initialised have to be set either using "name" or "ng-name"
attribute. For example:

  <div test foo="bar"></div>

will cause ElementBinder to register observer (which will in turn cause
initialisation) on property foo (of the directive) to bar, but if we try
using

  <div test></div>

foo property will not be initialised.

mvuksano · 2014-05-05T14:17:12Z

@mhevery Can you have a look and see if this makes sense? This is related to #981

mhevery · 2014-05-06T04:18:38Z

lib/core_dom/directive.dart

@@ -57,7 +57,9 @@ class NodeAttrs {
      if (_mustacheAttrs[attrName].isComputed) notifyFn(this[attrName]);
      _mustacheAttrs[attrName].notifyFn(true);
    } else {
-      notifyFn(this[attrName]);
+      if(element.attributes.keys.contains(attrName) || element.attributes.keys.contains('ng-$attrName') || attrName == 'name') {


Why are we special casing ng-??? Does not seem right.

.keys.contains() -> .containsKey()

mvuksano · 2014-05-12T23:40:20Z

@mhevery What do you think about "boolean attributes" (http://www.w3.org/TR/html4/intro/sgmltut.html#h-3.3.4.2)? Should we initialise them all with true, instead of node.attributes[attrName] (which is I guess null or an empty string)? I think we should do this too.

Actually, thinking more about this... I'm not sure if we can easily differentiate between
<div foo=""></div> - where foo is not a boolean attribute vs <div foo></div> where foo is a boolean attr.

jbdeboer · 2014-05-16T18:15:26Z

@markovuksanovic This change is ready to be submitted, yes?

mvuksano · 2014-05-17T04:26:03Z

@jbdeboer Yes, given the code is OK :) I just rebased it onto master.

vicb · 2014-05-23T09:23:57Z

lib/core_dom/directive.dart

@@ -57,7 +57,10 @@ class NodeAttrs {
      if (_mustacheAttrs[attrName].isComputed) notifyFn(this[attrName]);
      _mustacheAttrs[attrName].notifyFn(true);
    } else {
-      notifyFn(this[attrName]);
+      if(element.attributes.keys.contains(attrName)) {


containsKey ?

space after if

setting up observing. In case an element does not have a value set for a propertyv, that property is not automatically initialised. For example: <div test foo="bar"> will cause ElementBinder to register observer (which will in turn cause initialisation) on property foo (of the directive) to bar, but if we try using <div test> foo property will not be initialised.

mvuksano · 2014-05-26T10:58:37Z

class NgModel extends NgControl
fix attach() - call parent

jbdeboer · 2014-05-28T01:47:25Z

I'm hesitant about this PR. It is changing the component lifecycle but it unclear what problem it is solving.

I'm not sure that ``null` is the correct choice for an undefined situation. In many cases, components want to set default values for properties and now can't distinguish between a null that was explicitly passed to the component and a null that actually means that the attribute was not defined.

mvuksano · 2014-05-28T01:54:20Z

@jbdeboer It's supposed to solve issue #981.

Regarding null, if the attribute is undefined, maybe we should introduce an Undefined type and call the value with that type. But then we lose some type information...

setting up observing. In case an element does not have a value set for a propertyv, that property is not automatically initialised. For example: <div test foo="bar"> will cause ElementBinder to register observer (which will in turn cause initialisation) on property foo (of the directive) to bar, but if we try using <div test> foo property will not be initialised. Closes #1001

chirayuk · 2014-08-08T16:53:07Z

lib/core_dom/directive.dart

@@ -57,7 +57,10 @@ class NodeAttrs {
      if (_mustacheAttrs[attrName].isComputed) notifyFn(this[attrName]);
      _mustacheAttrs[attrName].notifyFn(true);
    } else {
-      notifyFn(this[attrName]);
+      if (element.attributes.containsKey(attrName)) {
+        var value = element.attributes[attrName];


Unused variable value?

mhevery · 2014-08-26T22:31:27Z

Closing, since this PR has perf implication. We really need to solve this in a unified way with bind-.

mvuksano added cla: yes and removed cla: no labels May 5, 2014

mhevery reviewed May 6, 2014
View reviewed changes

mvuksano changed the title ~~[WIP] feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when~~ feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when May 12, 2014

vicb reviewed May 23, 2014
View reviewed changes

mhevery added api: no-change labels Jul 25, 2014

chirayuk added the state: rebase label Aug 8, 2014

chirayuk reviewed Aug 8, 2014
View reviewed changes

jbdeboer removed the state: mergable label Aug 23, 2014

mhevery closed this Aug 26, 2014

rkirov mentioned this pull request Oct 10, 2014

Simply annotating a property with @NgAttr('foo') causes the setter to be called with null #981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when #1001

feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when #1001

mvuksano commented May 5, 2014

mvuksano commented May 5, 2014

mhevery May 6, 2014

vicb May 7, 2014

mvuksano commented May 12, 2014

jbdeboer commented May 16, 2014

mvuksano commented May 17, 2014

vicb May 23, 2014

vicb May 23, 2014

mvuksano commented May 26, 2014

jbdeboer commented May 28, 2014

mvuksano commented May 28, 2014

chirayuk Aug 8, 2014

mhevery commented Aug 26, 2014

feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when #1001

feat(NodeAttrs): Node attributes don't get initialized by NodeAttrs when #1001

Conversation

mvuksano commented May 5, 2014

mvuksano commented May 5, 2014

mhevery May 6, 2014

Choose a reason for hiding this comment

vicb May 7, 2014

Choose a reason for hiding this comment

mvuksano commented May 12, 2014

jbdeboer commented May 16, 2014

mvuksano commented May 17, 2014

vicb May 23, 2014

Choose a reason for hiding this comment

vicb May 23, 2014

Choose a reason for hiding this comment

mvuksano commented May 26, 2014

jbdeboer commented May 28, 2014

mvuksano commented May 28, 2014

chirayuk Aug 8, 2014

Choose a reason for hiding this comment

mhevery commented Aug 26, 2014