feat($compile): allow any attribute to use allOrNothing interpolation #8399

caitp · 2014-07-29T18:40:17Z

Previously, only a limited set of attributes would use allOrNothing
interpolation. It is now possible to request allOrNothing interpolation by
appending a '!' to the attribute name. This should result in attributes being
removed / not added to the node if their values are undefined, which can
prevent some kinds of errors in SVG elements.

Example:

<circle ng-attr-r!="{{dimensions.radius}}" x!="{{position.left}}">

Closes #8376

caitp · 2014-07-29T18:42:11Z

So we had some discussion about syntax like these, and people tend to not like having special meanings for certain characters, I'm really just bikeshedding here.

@btford / @IgorMinar / @petebacondarwin / whoever else, what do you think? I think it's good to be able to request allOrNothing declaratively, but I'm not sure about the syntax.

btford · 2014-07-29T18:52:17Z

I'm not sure about introducing new magical symbols in general.

I think != is especially confusing since it looks like "not equals." I wish we could use ≣, but that's kind of annoying to type.

I would like ng-attr-all-or-nothing-r="{{dimensions.radius}}" or some other (shorter) human-readable suffix.

caitp · 2014-07-29T18:54:57Z

It doesn't have to be a '!', it could certainly be ng-attr-somethingextra-<attr>, but preferably not super wordy :> --- it's also somewhat nicer to use a weird character like this because then you can generally use it for non-ng-attr-* too (I think it's unlikely for someone to use a "!" in their actual attribute name)

caitp · 2014-07-29T19:38:33Z

caitp did not add and remove the CLA labels! wat

btford · 2014-07-29T20:20:19Z

Oh GitHub you so crazy

petebacondarwin · 2014-07-30T08:12:59Z

I am pretty sure that was Igor's script - somehow pretending to be the original poster - you can see it in all the recent PRs.

This discussion is similar to the bind- discussions at the team meeting the other night. How was that resolved in the end?

petebacondarwin · 2014-07-30T08:18:02Z

How about just making allOrNothing the default on all attributes? Does it really make sense to allow an attribute to contain an interpolation in which some of the bits are undefined?

If you really needed your interpolations to support partially undefined interpolations then you could do:

my-attr="some static {{ possiblyUndefined || ''}  value"

or use a filter

my-attr="some static {{ possiblyUndefined | canBeUndefined ''}  value"

petebacondarwin · 2014-07-30T08:19:29Z

Also I don't think it really makes sense for the template writer to define this.

lgalfaso · 2014-07-30T11:35:50Z

src/ng/compile.js

+              if (name[name.length - 1] === '!') {
+                allOrNothing = true;
+                name = name.substring(0, name.length - 1);
+              }


This is weird, if the all-or-nothing property is checked when performing attribute scanning, then I would expect that this to works too for other directives that have a binding using '@'

It could be supported (depending on the syntax we go with) for @ too --- however lets figure out what the syntax should look like.

Do you have any preferences here @lgalfaso? @rodyhaddad?

Basically, there are two options:

it can be a suffix for the attribute name, but not a common attribute name (not alpha/numeric), such as !,*, ~, etc. This would allow it to be used after any attribute name.

it could precede the attribute name, and use a fatter/more human readable syntax, such as ng-attr-all-or-nothing-<name> --- it's a bit trickier to use this pattern for isolate scope bindings, and it's a bit more code to use this way, and obviously more typing to actually use this way. But it has the benefit of being more readable.

So, I'm not sure which I'd go with --- personally I'd prefer to make things easier for people, but realize that a "magic syntax" could get in peoples ways in some circumstances, rather than making things easier. So we should figure out how this should work.

I would go with a suffix that makes all browsers happy (with the exception of ! as it looks like != is a not equals) or with a prefix to the entire directive, this is all-or-nothing-ng-attr-foo="..." or all-or-nothing-my-binding-param="..." (where the all-or-nothing prefix is open to discussion)

IgorMinar · 2014-08-01T00:42:50Z

have we thought about what @petebacondarwin said? what if allornothing was the default?

are there valid cases that would break because of such change?

IgorMinar · 2014-08-01T00:52:39Z

alternatively we could make it the default for ng-attr-* though I know that ng-attr does not work for all svg use-cases (I think that this could be fixed though).

caitp · 2014-08-01T01:48:35Z

I'm okay with making it the default for ng-attr --- I don't think there are any real valid cases which would break

caitp · 2014-08-01T13:14:54Z

updated to remove the magic syntax and make it the default behaviour + added breaking change. LGTY?

allOrNothing interpolation is now used for ng-attr-*, under all circumstances. This prevents uninitialized attributes from being added to the DOM with invalid values which cause errors to be shown. BREAKING CHANGE: Now, ng-attr-* will never add the attribute to the DOM if any of the interpolated expressions evaluate to `undefined`. To work around this, initialize values which are intended to be the empty string with the empty string: For example, given the following markup: <div ng-attr-style="border-radius: {{value}}{{units}}"></div> If $scope.value is `4`, and $scope.units is undefined, the resulting markup is unchanged: <div ng-attr-style="border-radius: {{value}}{{units}}"></div> However, if $scope.units is `""`, then the resulting markup is updated: <div ng-attr-style="border-radius: {{value}}{{units}}" style="border-radius: 4"></div> Closes angular#8376

caitp · 2014-08-11T15:26:33Z

PTAL --- are we good to check this in for beta 18?

petebacondarwin · 2014-08-11T15:32:44Z

src/ng/compile.js

@@ -1958,7 +1958,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                // we need to interpolate again, in case the attribute value has been updated
                // (e.g. by another directive's compile function)
                interpolateFn = $interpolate(attr[name], true, getTrustedContext(node, name),
-                    ALL_OR_NOTHING_ATTRS[name]);
+                    ALL_OR_NOTHING_ATTRS[name] || allOrNothing);


This is probably pointless and pedantic but would it be slightly more performant to put the allOrNothing check first?

I'm not sure this will make a measurable difference, but in some cases putting the boolean first would win (when ng-attr-* is used frequently), in other cases it would lose (when href/src are used frequently). It would depend on the app, I guess

btford · 2014-08-11T15:34:09Z

LGTM

petebacondarwin · 2014-08-11T15:34:48Z

Other than my comment above, LGTM

caitp · 2014-08-11T15:43:50Z

thanks~

caitp added type: feature labels Jul 29, 2014

caitp added cla: yes and removed cla: no labels Jul 29, 2014

caitp mentioned this pull request Jul 30, 2014

ngAttr for boolean attributes #2111

Closed

lgalfaso reviewed Jul 30, 2014
View reviewed changes

Narretz added this to the Backlog milestone Aug 4, 2014

caitp modified the milestones: Backlog, 1.3.0-beta.19 Aug 10, 2014

petebacondarwin reviewed Aug 11, 2014
View reviewed changes

caitp closed this in 09de7b5 Aug 21, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): allow any attribute to use allOrNothing interpolation #8399

feat($compile): allow any attribute to use allOrNothing interpolation #8399

caitp commented Jul 29, 2014

caitp commented Jul 29, 2014

btford commented Jul 29, 2014

caitp commented Jul 29, 2014

caitp commented Jul 29, 2014

btford commented Jul 29, 2014

petebacondarwin commented Jul 30, 2014

petebacondarwin commented Jul 30, 2014

petebacondarwin commented Jul 30, 2014

lgalfaso Jul 30, 2014

caitp Jul 30, 2014

caitp Jul 30, 2014

lgalfaso Jul 30, 2014

IgorMinar commented Aug 1, 2014

IgorMinar commented Aug 1, 2014

caitp commented Aug 1, 2014

caitp commented Aug 1, 2014

caitp commented Aug 11, 2014

petebacondarwin Aug 11, 2014

caitp Aug 11, 2014

btford commented Aug 11, 2014

petebacondarwin commented Aug 11, 2014

caitp commented Aug 11, 2014

feat($compile): allow any attribute to use allOrNothing interpolation #8399

feat($compile): allow any attribute to use allOrNothing interpolation #8399

Conversation

caitp commented Jul 29, 2014

caitp commented Jul 29, 2014

btford commented Jul 29, 2014

caitp commented Jul 29, 2014

caitp commented Jul 29, 2014

btford commented Jul 29, 2014

petebacondarwin commented Jul 30, 2014

petebacondarwin commented Jul 30, 2014

petebacondarwin commented Jul 30, 2014

lgalfaso Jul 30, 2014

Choose a reason for hiding this comment

caitp Jul 30, 2014

Choose a reason for hiding this comment

caitp Jul 30, 2014

Choose a reason for hiding this comment

lgalfaso Jul 30, 2014

Choose a reason for hiding this comment

IgorMinar commented Aug 1, 2014

IgorMinar commented Aug 1, 2014

caitp commented Aug 1, 2014

caitp commented Aug 1, 2014

caitp commented Aug 11, 2014

petebacondarwin Aug 11, 2014

Choose a reason for hiding this comment

caitp Aug 11, 2014

Choose a reason for hiding this comment

btford commented Aug 11, 2014

petebacondarwin commented Aug 11, 2014

caitp commented Aug 11, 2014