Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

amp-bind: Support data-amp-bind-* attributes #15408

Merged
merged 2 commits into from
May 19, 2018
Merged

amp-bind: Support data-amp-bind-* attributes #15408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae5cf40
Support data-amp-bind-*, fix random lint and type errors.
May 17, 2018
ed5edf2
Ignore data-amp-bind-foo if [foo] already exists, add tests.
May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 56 additions & 60 deletions extensions/amp-bind/0.1/bind-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {BindExpressionResultDef} from './bind-expression';
import {BindValidator} from './bind-validator';
import {BindingDef} from './bind-evaluator';
import {ChunkPriority, chunk} from '../../../src/chunk';
import {Deferred} from '../../../src/utils/promise';
import {RAW_OBJECT_ARGS_KEY} from '../../../src/action-constants';
import {Services} from '../../../src/services';
import {debounce} from '../../../src/utils/rate-limit';
Expand All @@ -36,6 +37,7 @@ import {map} from '../../../src/utils/object';
import {parseJson, recursiveEquals} from '../../../src/json';
import {reportError} from '../../../src/error';
import {rewriteAttributesForElement} from '../../../src/sanitizer';
import {startsWith} from '../../../src/string';

const TAG = 'amp-bind';

Expand All @@ -55,23 +57,21 @@ const MAX_MERGE_DEPTH = 10;
/**
* A bound property, e.g. [property]="expression".
* `previousResult` is the result of this expression during the last evaluation.
* @typedef {{
* property: string,
* expressionString: string,
* previousResult: (./bind-expression.BindExpressionResultDef|undefined),
* }}
* @typedef {{property: string, expressionString: string, previousResult: (./bind-expression.BindExpressionResultDef|undefined)}}
*/
let BoundPropertyDef;

/**
* A tuple containing a single element and all of its bound properties.
* @typedef {{
* boundProperties: !Array<BoundPropertyDef>,
* element: !Element,
* }}
* @typedef {{boundProperties: !Array<BoundPropertyDef>, element: !Element}}
*/
let BoundElementDef;

/**
* @typedef {{boundElements: !Array<BoundElementDef>, bindings: !Array<./bind-evaluator.BindingDef>, expressionToElements: !Object<string, !Array<!Element>>, limitExceeded: boolean}}
*/
let NodeScanDef;

/**
* A map of tag names to arrays of attributes that do not have non-bind
* counterparts. For instance, amp-carousel allows a `[slide]` attribute,
Expand Down Expand Up @@ -547,7 +547,6 @@ export class Bind {
* @param {!Array<!Node>} nodes
* @return {!Promise}
* @private
* @visibleForTesting
*/
removeBindingsForNodes_(nodes) {
const before = (getMode().development) ? this.numberOfBindings() : 0;
Expand Down Expand Up @@ -600,14 +599,7 @@ export class Bind {
* a tuple containing bound elements and binding data for the evaluator.
* @param {!Node} node
* @param {number} limit
* @return {
* !Promise<{
* boundElements: !Array<BoundElementDef>,
* bindings: !Array<./bind-evaluator.BindingDef>,
* expressionToElements: !Object<string, !Array<!Element>>,
* limitExceeded: boolean,
* }>
* }
* @return {!Promise<NodeScanDef>}
* @private
*/
scanNode_(node, limit) {
Expand Down Expand Up @@ -657,34 +649,34 @@ export class Bind {
return !walker.nextNode() || limitExceeded;
};

return new Promise(resolve => {
const chunktion = idleDeadline => {
let completed = false;
// If `requestIdleCallback` is available, scan elements until
// idle time runs out.
if (idleDeadline && !idleDeadline.didTimeout) {
while (idleDeadline.timeRemaining() > 1 && !completed) {
completed = scanNextNode_();
}
} else {
// If `requestIdleCallback` isn't available, scan elements in buckets.
// Bucket size is a magic number that fits within a single frame.
const bucketSize = 250;
for (let i = 0; i < bucketSize && !completed; i++) {
completed = scanNextNode_();
}
const {promise, resolve} = new Deferred();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const chunktion = idleDeadline => {
let completed = false;
// If `requestIdleCallback` is available, scan elements until
// idle time runs out.
if (idleDeadline && !idleDeadline.didTimeout) {
while (idleDeadline.timeRemaining() > 1 && !completed) {
completed = scanNextNode_();
}
// If we scanned all elements, resolve. Otherwise, continue chunking.
if (completed) {
resolve({
boundElements, bindings, expressionToElements, limitExceeded,
});
} else {
chunk(this.ampdoc, chunktion, ChunkPriority.LOW);
} else {
// If `requestIdleCallback` isn't available, scan elements in buckets.
// Bucket size is a magic number that fits within a single frame.
const bucketSize = 250;
for (let i = 0; i < bucketSize && !completed; i++) {
completed = scanNextNode_();
}
};
chunk(this.ampdoc, chunktion, ChunkPriority.LOW);
});
}
// If we scanned all elements, resolve. Otherwise, continue chunking.
if (completed) {
resolve({
boundElements, bindings, expressionToElements, limitExceeded,
});
} else {
chunk(this.ampdoc, chunktion, ChunkPriority.LOW);
}
};
chunk(this.ampdoc, chunktion, ChunkPriority.LOW);
return promise;
}

/**
Expand Down Expand Up @@ -715,15 +707,26 @@ export class Bind {
* @private
*/
scanAttribute_(attribute, element) {
const {tagName} = element;
const {name} = attribute;
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per #15204 (comment), isn't the old way better? const {name, tagName } = element?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use the shorter tag variable for conciseness.

const attr = attribute.name;

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-')) {
Copy link
Contributor

@aghassemi aghassemi May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

property = attr.substr(14);
// Ignore `data-amp-bind-foo` if `[foo]` already exists.
if (element.hasAttribute(`[${property}]`)) {
property = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: just return here.

}
}

if (property) {
if (this.validator_.canBind(tag, property)) {
return {property, expressionString: attribute.value};
} else {
const err = user().createError(
`${TAG}: Binding to [${property}] on <${tagName}> is not allowed.`);
`${TAG}: Binding to [${property}] on <${tag}> is not allowed.`);
reportError(err, element);
}
}
Expand Down Expand Up @@ -754,9 +757,7 @@ export class Bind {

/**
* Reevaluates all expressions and returns a map of expressions to results.
* @return {!Promise<
* !Object<string, ./bind-expression.BindExpressionResultDef>
* >}
* @return {!Promise<!Object<string, ./bind-expression.BindExpressionResultDef>>}
* @private
*/
evaluate_() {
Expand Down Expand Up @@ -805,12 +806,7 @@ export class Bind {
* new value.
* @param {!Array<!BoundPropertyDef>} boundProperties
* @param {Object<string, ./bind-expression.BindExpressionResultDef>} results
* @return {
* !Array<{
* boundProperty: !BoundPropertyDef,
* newValue: !./bind-expression.BindExpressionResultDef,
* }>
* }
* @return {!Array<{boundProperty: !BoundPropertyDef, newValue: !./bind-expression.BindExpressionResultDef}>}
* @private
*/
calculateUpdates_(boundProperties, results) {
Expand Down Expand Up @@ -928,7 +924,7 @@ export class Bind {
* @param {!BoundPropertyDef} boundProperty
* @param {!Element} element
* @param {./bind-expression.BindExpressionResultDef} newValue
* @return (?{name: string, value:./bind-expression.BindExpressionResultDef})
* @return {?{name: string, value:./bind-expression.BindExpressionResultDef}}
* @private
*/
applyBinding_(boundProperty, element, newValue) {
Expand Down
21 changes: 21 additions & 0 deletions extensions/amp-bind/0.1/test/integration/test-bind-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,27 @@ describe.configure().ifNewChrome().run('Bind', function() {
});
});

it('should support data-amp-bind-* syntax', () => {
const element = createElement(env, container, 'data-amp-bind-text="1+1"');
expect(bind.numberOfBindings()).to.equal(0);
expect(element.textContent).to.equal('');
return onBindReadyAndSetState(env, bind, {}).then(() => {
expect(bind.numberOfBindings()).to.equal(1);
expect(element.textContent).to.equal('2');
});
});

it('should prefer [foo] over data-amp-bind-foo', () => {
const element = createElement(
env, container, '[text]="1+1" data-amp-bind-text="2+2"');
expect(bind.numberOfBindings()).to.equal(0);
expect(element.textContent).to.equal('');
return onBindReadyAndSetState(env, bind, {}).then(() => {
expect(bind.numberOfBindings()).to.equal(1);
expect(element.textContent).to.equal('2');
});
});

it('should call createTreeWalker() with all params', () => {
const spy = env.sandbox.spy(env.win.document, 'createTreeWalker');
createElement(env, container, '[text]="1+1"');
Expand Down