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.

Sign up for GitHub

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

Jump to bottom

amp-toolbox/optimizer should set AMP_CONFIG URLs when self-hosting AMP JS #24927

Closed
mattwomple opened this issue Sep 25, 2019 · 11 comments
Closed

amp-toolbox/optimizer should set AMP_CONFIG URLs when self-hosting AMP JS #24927

mattwomple opened this issue Sep 25, 2019 · 11 comments
Assignees
@dreamofabear

Comments

@mattwomple
Copy link
Member

mattwomple commented Sep 25, 2019 •
edited
Loading

What's the issue?

When AMP pages that have been optimized using the amp-toolbox transformer are loaded into a PWA using shadow-v0.js, any self-hosted AMP runtime URLs for AMP extensions are ignored and cdn.ampproject.org is used instead.

How do we reproduce the issue?

Edited 2019-10-07 with much simpler sample.

A minimal example PWA that illustrates this issue is available at:

These HTML files, along with the simple node.js script that transformed -amp.html to -opt.html is available in github.com/mattwomple/self-hosted-amp-pwa.

Watch the network traffic when loading self-hosted-amprt-amp.html. You should see v0.js and amp-form-0.1.js are requested from https://amp-android.cmphys.com/7769/amp-dist/. Note: amp-auto-lightbox-0.1.js loads from cdn.ampproject.org because it is added dynamically; this is another issue for another day.

Next, watch the network traffic when loading self-hosted-amprt-pwa.html. Notice shadow-v0.js is requested from https://amp-android.cmphys.com/7769/amp-dist/ but amp-form-0.1.js is requested from https://cdn.ampproject.org/rtv/011909141409590/v0/. This is what I hope can be fixed.

What browsers are affected?

All browsers. Tested on Chrome 77.

Which AMP version is affected?

1909141409590
@dreamofabear
Copy link

Hm, do we suggest self-hosting AMP JS anywhere e.g. in documentation? This may be uncharted territory.

This can technically be done by prepending a custom self.AMP_CONFIG = {...} into the self-hosted binaries. This is what other AMP Caches do to change cdn.ampproject.org references in runtime code, for example. See https://github.com/ampproject/amphtml/blob/master/src/config.js.

@mattwomple
Copy link
Member Author

The amp-toolbox optimizer explains the basics of self-hosting here: https://github.com/ampproject/amp-toolbox/tree/master/packages/optimizer#self-hosted-amp-components

Setting ampUrlPrefix works well for AMP pages. It's only when the AMP page is viewed in a PWA that the URLs in the AMP document to AMP components are not respected. I consider this a bug.

@dreamofabear
Copy link

Thanks. Sounds like the optimizer should be prepending AMP_CONFIG when ampUrlPrefix is provided. I'll transfer this bug to the amp-toolbox repository.

/cc @sebastianbenz

@dreamofabear
Copy link

Or @sebastianbenz can do it because I don't have access. :)

@dreamofabear dreamofabear changed the title Shadow v0 ignores self-hosted AMP runtime for AMP extensions amp-toolbox/optimizer should set AMP_CONFIG URLs when self-hosting AMP JS Oct 4, 2019
@mattwomple
Copy link
Member Author

Thanks. Sounds like the optimizer should be prepending AMP_CONFIG when ampUrlPrefix is provided. I'll transfer this bug to the amp-toolbox repository.

/cc @sebastianbenz

Thank you for the tip @choumx . Could you point me to an AMP page that has AMP_CONFIG defined in its HTML? I may be able to work on this bugfix in amp-toolbox if I know where AMP_CONFIG is expected to live.

@mattwomple
Copy link
Member Author

mattwomple commented Oct 5, 2019 •
edited
Loading

I haven't solved this puzzle yet. It looks to me like AMP_CONFIG is set by shadow-v0.js first thing, overwriting anything that already exists in window.AMP_CONFIG. Modifications to AMP_CONFIG.cdnUrl and AMP_CONFIG.cdnProxyRegex before or after shadow-v0.js runs are ignored. I'm not making the connection to amp-toolbox's responsibility in all this either.

The updated minimal example that was previously here was moved to the original post on 2019-10-07

@dreamofabear dreamofabear self-assigned this Oct 7, 2019
@sebastianbenz sebastianbenz transferred this issue from ampproject/amphtml Oct 7, 2019
@sebastianbenz sebastianbenz transferred this issue from ampproject/amp-toolbox Oct 7, 2019
@sebastianbenz
Copy link
Contributor

Thanks for filing this Matt! This will also become relevant once we will officially support self-hosting the runtime. It'd be nice to have a way to do this that doesn't require rewriting the runtime bundles. Maybe <meta name="runtime-host" content="example.com">? We could ensure via the validator that this is only allowed if <html transformed="XXXXX"> is set.

//cc @honeybadgerdontcare

P.S. Sorry, for the issue transferral noise (should've read to the end first).

@dreamofabear
Copy link

@mattwomple Right, you'd need to overwrite the self.AMP_CONFIG={...} line not simply prepend. Sorry for the confusion.

@mattwomple mattwomple mentioned this issue Oct 13, 2019
Closed
@mattwomple
Copy link
Member Author

mattwomple commented Oct 13, 2019 •
edited
Loading

The bulk of my original issue report is remedied by setting an environment variable before loading shadow-v0.js:

<script>
  window.AMP_CONFIG = {
    cdnUrl: 'https://amp-android.cmphys.com/7769/amp-dist'
  };
</script>

PR #25026 brings amphtml's extension URL generation in-line with amp-toolbox/optimizer.
PR #25028 adds support for updating AMP.config.urls in singleDoc mode.

@mattwomple mattwomple mentioned this issue Oct 14, 2019
Closed
@dreamofabear
Copy link

That's a good workaround.

@mattwomple mattwomple mentioned this issue Nov 19, 2019
Closed
@mattwomple
Copy link
Member Author

Closing in favor of #25873.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dreamofabear dreamofabear

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sebastianbenz @dreamofabear @mattwomple