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.

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

Add amp-bind optimizer #1147

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Add amp-bind optimizer #1147

merged 3 commits into from
Feb 26, 2021

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Feb 25, 2021

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Nice!

Two comments:

packages/optimizer/lib/DomTransformer.js Show resolved Hide resolved
packages/optimizer/lib/DomTransformer.js Show resolved Hide resolved
packages/optimizer/lib/transformers/OptimizeAmpBind.js Outdated Show resolved Hide resolved
this.log_ = config.log.tag('OptimizeAmpBind');

// setup implementation only if placeholder generation is enabled
this.enabled_ = !!config.optimizeAmpBind;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should enable this by default.

Suggested change
this.enabled_ = !!config.optimizeAmpBind;
this.enabled_ = config.optimizeAmpBind !== false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or did you intentionally leave this disabled until the validator changes are live?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@sebastianbenz
Copy link
Collaborator

//cc @westonruter @schlessera for porting this to the PHP optimizer.

Copy link
Contributor Author

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

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

Working on that now.

this.log_ = config.log.tag('OptimizeAmpBind');

// setup implementation only if placeholder generation is enabled
this.enabled_ = !!config.optimizeAmpBind;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@jridgewell
Copy link
Contributor Author

Tests added

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants