amp-bind: Support data-amp-bind-* attributes #15408
Conversation
dreamofabear
force-pushed
the
bind-alternate-syntax
branch
from
f68f624
to
b23bdd3
Compare
| if (name.length > 2 && name[0] === '[' && name[name.length - 1] === ']') { | ||
| const property = name.substr(1, name.length - 2); | ||
| if (this.validator_.canBind(tagName, property)) { | ||
| const tag = element.tagName; |
There was a problem hiding this comment.
per #15204 (comment), isn't the old way better? const {name, tagName } = element?
There was a problem hiding this comment.
I wanted to use the shorter tag variable for conciseness.
| let property; | ||
| if (attr.length > 2 && attr[0] === '[' && attr[attr.length - 1] === ']') { | ||
| property = attr.substr(1, attr.length - 2); | ||
| } else if (startsWith(attr, 'data-amp-bind-')) { |
There was a problem hiding this comment.
interesting, I initially thought this is allowing any data-amp-bind-* to become bindable via [data-amp-bind-*] ( I actually ran into a case few days ago that I wanted to use a data- attribute in CSS instead of class) but I see that this is an alternative to [foo].
Given we may want to allow arbitrary data- attributes to become bindable in the feature (like how we do with aria-*), I wonder if we should remove data- prefix from this one and make it amp-bind-* and add a general validation/sanitizer rule for it.
There was a problem hiding this comment.
Updated the PR description to make this more clear.
It's still safe for binding to data-* attributes since the expected prefix is the more specific data-amp-bind-*. I don't think we'll ever want to support something like [data-amp-bind-foo].
You're right that the validator won't enforce rules for data-amp-bind-*, but I think this is fine for a "workaround" syntax like this one. It'll also avoid yet another source of skew (between validator rules for [foo] vs. data-amp-bind-foo vs. bind-validator.js).
| if (name.length > 2 && name[0] === '[' && name[name.length - 1] === ']') { | ||
| const property = name.substr(1, name.length - 2); | ||
| if (this.validator_.canBind(tagName, property)) { | ||
| const tag = element.tagName; |
There was a problem hiding this comment.
I wanted to use the shorter tag variable for conciseness.
| let property; | ||
| if (attr.length > 2 && attr[0] === '[' && attr[attr.length - 1] === ']') { | ||
| property = attr.substr(1, attr.length - 2); | ||
| } else if (startsWith(attr, 'data-amp-bind-')) { |
There was a problem hiding this comment.
Updated the PR description to make this more clear.
It's still safe for binding to data-* attributes since the expected prefix is the more specific data-amp-bind-*. I don't think we'll ever want to support something like [data-amp-bind-foo].
You're right that the validator won't enforce rules for data-amp-bind-*, but I think this is fine for a "workaround" syntax like this one. It'll also avoid yet another source of skew (between validator rules for [foo] vs. data-amp-bind-foo vs. bind-validator.js).
dreamofabear
force-pushed
the
bind-alternate-syntax
branch
from
b23bdd3
to
ed5edf2
Compare
| for (let i = 0; i < bucketSize && !completed; i++) { | ||
| completed = scanNextNode_(); | ||
| } | ||
| const {promise, resolve} = new Deferred(); |
There was a problem hiding this comment.
Nit: Did the linter ask you to convert this? It was actually better the other way, because any sync error throws would have been caught.
| property = attr.substr(14); | ||
| // Ignore `data-amp-bind-foo` if `[foo]` already exists. | ||
| if (element.hasAttribute(`[${property}]`)) { | ||
| property = null; |
…9835) * Add docs for XML-compatible binding attribute syntax See ampproject#11115 and ampproject#15408 * Minor edits
Fixes #11115. Also fixes a few lint errors.
data-amp-bind-foo="bar"as an alternative to[foo]="bar".Performance impact of additional string operations seems negligible. Time spent scanning
examples/bind/performance.amp.htmlon Mac Pro 2013 with 6x CPU throttling (10 samples each):/to @aghassemi