amp-bind: Expression complexity limit#8321
Conversation
3a4e204 to
c1ba131
Compare
|
/to @aghassemi @kmh287 PTAL |
| const size = this.numberOfNodesInAst_(this.ast_); | ||
| const maxSize = opt_maxAstSize || DEFAULT_MAX_AST_SIZE; | ||
| const skipConstraint = getMode().localDev && !getMode().test; | ||
| if (size > maxSize && !skipConstraint) { |
There was a problem hiding this comment.
This is going to be the message printed in the console right? If so, I'm wondering if we should give more context here. Someone unfamiliar with the amp-bind implementation might not know what to make of this message.
There was a problem hiding this comment.
Actually this error will eventually be passed to reportError(), which highlights the element that contains this expression in dev console.
There was a problem hiding this comment.
Nonetheless, I'm not sure that the error gives a clear picture of the problem to the uninitiated. Maybe add something like "Expression contains too many literals, variables, or function calls". Up to you.
There was a problem hiding this comment.
Oh I see, I thought you meant contextual location rather than content. Done!
| /** @const @private {!./bind-expr-defines.AstNode} */ | ||
| this.ast_ = parser.parse(this.expressionString); | ||
|
|
||
| // Check if this expression string is too large (for performance). |
There was a problem hiding this comment.
Just curious: what's the performance impact of doing this?
There was a problem hiding this comment.
Good question. Looks like a few ms in total on examples/bind/performance.amp.html with 5x throttling.
|
Thanks for the reviews! |
| // Check if this expression string is too large (for performance). | ||
| const size = this.numberOfNodesInAst_(this.ast_); | ||
| const maxSize = opt_maxAstSize || DEFAULT_MAX_AST_SIZE; | ||
| const skipConstraint = getMode().localDev && !getMode().test; |
There was a problem hiding this comment.
This needs a warning if we're in dev mode and we make it too big.
* master: (34 commits) Prevent amp-carousel next/previous icons fade away on desktop (ampproject#8428) Turn on flag slidescroll-disable-css-snap” (ampproject#8436) Revert "temporarily turn off yarn (ampproject#8356)" (ampproject#8384) initial commit (ampproject#8404) Upgrades for Index Exchange amp-ad tags to report load statistics (ampproject#8054) amp-bind validation tweak (ampproject#8414) Fix an amp-instagram race condition (ampproject#8192) Use whitelist to restrict urlReplacement for scoped analytics element (ampproject#8360) Report active experiments in error logs (ampproject#8108) amp-bind: Catch exceptions in mutatedAttributesCallback (ampproject#8383) Fixing custom scroll-snap on IOS (ampproject#8391) Add experiment for using AmpContext class in integration.js (ampproject#8348) add (ampproject#8349) swipe api (ampproject#8357) skip 3 flaky tests (ampproject#8388) amp-bind: Expression complexity limit (ampproject#8321) add margin-bottom (ampproject#8350) Flying carpet: make container full viewport and center content (ampproject#8292) Service Registration: Document Click (ampproject#7882) Add a8ad (ampproject#8036) ...
* initial commit for expr size constraint * rename test-bind-expr.js to test-bind-expression.js * unit test * ignore constraint in localDev * also ignore binding constraint in localDev * don't skip constraints while testing * improve error message
Partial for #8057.
examples/bind/performance.amp.html.