Skip to content

optimizer: Add lts option to URL rewriter#691

Merged
sebastianbenz merged 2 commits intoampproject:masterfrom
mdmower:pr-lts
Apr 15, 2020
Merged

optimizer: Add lts option to URL rewriter#691
sebastianbenz merged 2 commits intoampproject:masterfrom
mdmower:pr-lts

Conversation

@mdmower
Copy link
Copy Markdown
Contributor

@mdmower mdmower commented Apr 5, 2020

Note that lts option is incompatible with specifying a specific runtime version in URL rewrites. An error will be thrown if these options are used together.

Depends on #695
Fixes #629

Support optimizing AMP pages to use long-term stable URLs:
https://github.com/ampproject/amphtml/blob/master/contributing/lts-release.md

Update CLI with support for all current rewrite URLs options.

Note that lts option is incompatible with specifying a custom host or a
specific runtime version in URL rewrites. An error will be thrown if
these options are used together.
@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Apr 5, 2020

Not quite ready. Need to update AmpBoilerplateTransformer to use ltsCssUrl from https://cdn.ampproject.org/rtv/metadata. Would like to wait for #695 and #697 before making changes in AmpBoilerplateTransformer.

@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Apr 11, 2020

Depends on #695
Ready for review.

Copy link
Copy Markdown
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 a lot!

}
// TODO: If ampUrlPrefix is a relative URL, this will fall back to
// fetching the latest runtime version and boilerplate CSS from
// cdn.ampproject.org. Is this our best option?
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Another option would be to support passing v0.css via a parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I left this fallback in place so that this PR doesn't affect handling of relative ampUrlPrefix. I think we're both on-board with using absolute URLs for ampUrlPrefix going forward, so this will disappear in a future PR. Can we keep this as-is just for the purposes of this PR?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Feature request: Support LTS as runtime version in AMP Optimizer

3 participants