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

ITI: Asset URL rewriting in AMP Optimizer #862

Open
jonluca opened this issue Jul 10, 2020 · 4 comments
Open

ITI: Asset URL rewriting in AMP Optimizer #862

jonluca opened this issue Jul 10, 2020 · 4 comments

Comments

@jonluca
Copy link

jonluca commented Jul 10, 2020

Moving the conversation here based on ampproject/amppackager#449

We’d like to move forward with adding AMP Packager compatibility mode to the AMP optimizer.

From the previous issue:

The important thing to note is that in the near future, transformers won't need to produce byte compatible AMPHTML to the AMPPackager. This will greatly simplify things and means that transformers only need to produce valid AMP. So you will be able to use AMP Optimizer out of the box. However, for performance reasons, asset URL rewriting to cdn.ampproject.org is critical in the SXG case and should be added to AMP Optimizer. This could be enabled via a flag as an "AMPPacker compatibility mode".

We feel comfortable in adding in a new flag ( --amp-packager-mode?). The base URL would default to cdn.ampproject.org, but could be changed to support different AMP caches (is this what the AMP-Cache-Transform request header is for?).

@sebastianbenz
Copy link
Collaborator

Sounds good! I'd prefer to name the flag more generic though: rewriteUrlsToAmpCache=cdn.ampproject.org?

One note: rewriting script import URLs is already supported via the ampUrlPrefix flag. If the packager flag is set this should be overridden (and print a warning if it is defined as well).

@jonluca
Copy link
Author

jonluca commented Jul 13, 2020

Is there a reason we shouldn't just use the ampUrlPrefix flag? It seems like the behavior is going to be the same. Is ampUrlPrefix used elsewhere and shouldn't be overloaded?

Also, does a mapping of amp-cache-transform headers to URLs exist somewhere already? Took a look in the amp-packager repo but didn't see anything of note.

@sebastianbenz
Copy link
Collaborator

ampUrlPrefix is meant for runtime self-hosting. In that case you want to rewrite all script srcs, but not all asset links.

I'm not aware of an existing amp-cache-transform header mapping - afaik, the Google AMP Cache is the only one supporting SXG.

@twifkak
Copy link
Member

twifkak commented Jul 16, 2020

The amp-cache-transform header mapping is caches.json, as specified (though I'm open to PRs against the spec). For ease of implementation, this is also mirrored at https://cdn.ampproject.org/caches.json.

In practice, AMP Packager is hard-coded to support only google but that's an open bug. No need to reproduce that buggy behavior.

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

No branches or pull requests

3 participants