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

rewrite_domains doesn't apply to static assets #1350

Closed
oschaaf opened this Issue Jul 14, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@oschaaf
Copy link
Member

oschaaf commented Jul 14, 2016

Scripts written into the html by PSOL for (for example) defer_javascript and lazyload_images cannot be rewritten to be served via a CDN.

From https://groups.google.com/d/msgid/mod-pagespeed-discuss/fd26cdb8-f4bc-4629-96cd-dd604b5b1649%40googlegroups.com:

  • There's the StaticAssetPrefix [1] option, which takes an absolute path.
    I did attempt to specify a full domain in this option instead of just that. The option allows doing so and it will be used in the html output correctly, but it looks like mapping the resulting incoming requests fails.
  • The MapRewriteDomain option does not apply to pagespeed's static assets.
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

I think MapRewriteDomain should be applying to the static assets. I'll figure out why not.

@jeffkaufman jeffkaufman self-assigned this Jul 14, 2016

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

Reproduced:

<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
  ServerName map-static-domain.example.com
  DocumentRoot "@@APACHE_DOC_ROOT@@"
  ModPagespeedFileCachePath "@@MOD_PAGESPEED_CACHE@@"

  ModPagespeedMapRewriteDomain http://static-cdn.example.com \
                               http://map-static-domain.example.com

  ModPagespeedRewriteLevel PassThrough
  ModPagespeedEnableFilters defer_javascript
</VirtualHost>

http_proxy=localhost:8083 curl -sS -D- -H 'User-Agent: Chrome/100' \
  map-static-domain.example.com/mod_pagespeed_example/rewrite_javascript.html \
  | grep /mod_pagespeed_static/
...<script type="text/javascript" src="/mod_pagespeed_static/js_defer.tRzI7EHUpg.js"></script></body>

That should have read http://static-cdn.example.com/mod_pagespeed_static/js_defer.tRzI7EHUpg.js

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

I'm not sure whether you should need to have to turn on rewrite_domains for this to work.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

I see two ways for this to work:

  • StaticAssetManager::GetAssetUrl could call DomainRewriteFilter::Rewrite like the css tag scanner does. This is awkward because currently GetAssetUrl doesn't know what the base is.
  • DomainRewriteFilter::Rewrite could allow rewriting .pagespeed. resources that are static assets. I think this would be a pretty tidy change, but it would require people to turn on rewrite_domains if they want this behavior, which is a little strange.
@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Jul 14, 2016

There is also PageSpeedStaticAssetCDN, but that might require one to
actually distribute the files, which is probably impractical for
normal users.
(It was added for mobilizer experiments...)

See TEST_F(SystemRewriteOptionsTest, StaticAssetCdn) for the closest
it has to documentation

On Thu, Jul 14, 2016 at 8:56 AM, Jeff Kaufman notifications@github.com wrote:

I see two ways for this to work:

StaticAssetManager::GetAssetUrl could consult the lawyer and apply the
mappings. This is awkward because (1) calling the mapping code is somewhat
complex (see DomainRewriteFilter::Rewrite) and also because currently
GetAssetUrl doesn't know what the base is.
DomainRewriteFilter::Rewrite could allow rewriting .pagespeed. resources
that are static assets. I think this would be a pretty tidy change, but it
would require people to turn on rewrite_domains if they want this behavior,
which is a little strange.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

Actually, I think all we need to do is move rewrite domains to run after everything that inserts static assets

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 14, 2016

change is out for review

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Aug 2, 2016

@jeffkaufman jeffkaufman changed the title Facilitate rewriting the domain of pagespeed's static assets rewrite_domains should apply to PageSpeed's static assets Oct 3, 2016

@jeffkaufman jeffkaufman changed the title rewrite_domains should apply to PageSpeed's static assets rewrite_domains doesn't apply to static assets Oct 11, 2016

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