Add amp-bind optimizer#1147
Conversation
sebastianbenz
left a comment
There was a problem hiding this comment.
Nice!
Two comments:
- using
data-amphtml-bindinginstead ofI-amphtml-bindingwould mean that we wouldn't have to update the validator - can you please add a test here: https://github.com/ampproject/amp-toolbox/tree/main/packages/optimizer/spec/transformers/valid. You can run
npm run optimier:test:snapshotto generate the expected output files
| this.log_ = config.log.tag('OptimizeAmpBind'); | ||
|
|
||
| // setup implementation only if placeholder generation is enabled | ||
| this.enabled_ = !!config.optimizeAmpBind; |
There was a problem hiding this comment.
We should enable this by default.
| this.enabled_ = !!config.optimizeAmpBind; | |
| this.enabled_ = config.optimizeAmpBind !== false; |
There was a problem hiding this comment.
Or did you intentionally leave this disabled until the validator changes are live?
There was a problem hiding this comment.
Yup, this is intentionally disabled until validator changes are live in AMP Cache. I was gonna follow up with a PR to enable it afterwards.
|
//cc @westonruter @schlessera for porting this to the PHP optimizer. |
Co-authored-by: Sebastian Benz <sebastian.benz@gmail.com>
jridgewell
left a comment
There was a problem hiding this comment.
- using data-amphtml-binding instead of I-amphtml-binding would mean that we wouldn't have to update the validator
I actually chose not to do this, since the attribute can only be valid in transformed outputs. Data attributes are always valid.
- can you please add a test here: https://github.com/ampproject/amp-toolbox/tree/main/packages/optimizer/spec/transformers/valid. You can run npm run optimier:test:snapshot to generate the expected output files
Working on that now.
| this.log_ = config.log.tag('OptimizeAmpBind'); | ||
|
|
||
| // setup implementation only if placeholder generation is enabled | ||
| this.enabled_ = !!config.optimizeAmpBind; |
There was a problem hiding this comment.
Yup, this is intentionally disabled until validator changes are live in AMP Cache. I was gonna follow up with a PR to enable it afterwards.
|
Tests added |
See ampproject/amphtml#32851
Part of ampproject/amphtml#27590