search

apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 365 forks source link

JavaScript files not getting rewritten (optimized) #873

Closed nikolay closed 9 years ago

nikolay commented 9 years ago

Some files are never optimized by PageSpeed and it's not clear why. I understand that it skips the ones with "use strict" although those would be a growing number, but this is not the case with the following scripts:

These scripts are referenced here: http://www.vocativ.com/

As you can see on this page, other scripts get rewritten with a hash, which is the behavior I look for, and then their domains are rewritten as well to use the CDN (www1 and www2). Examples:

So, if I file changes in a release, I don't have to purge the CDN, because the hash changes and there won't be collisions. Unfortunately, PageSpeed selectively skips some files and it's not clear to me why.

I'm using version 1.8.31.4 of PageSpeed.

oschaaf commented 9 years ago

@nikolay http://www.vocativ.com/content/mu-plugins/vocativ-newsletter-modal/js/vocativ-newsletter-modal.js looks like it's introspective. You could try if this helps, but if you do that you need to test if nothing gets broken:

pagespeed AvoidRenamingIntrospectiveJavascript off;

Also, see https://developers.google.com/speed/pagespeed/module/restricting_urls#aris

oschaaf commented 9 years ago

P.s. the same applies to http://www2.vocativ.com/content/themes/vocativ/js/jquery.fitvids.js

jeffkaufman commented 9 years ago

(I'm wondering whether we should turn off the ARIS heuristic by default. When implementing it we had thought it wasn't very common and was used mostly in cases where renaming the js would be a problem, but it's actually used in a lot of other cases. For example, one way to make a script asynchronous (and the one Google Analytics uses triggers our detector. I remember looking over a random sample of js that triggered this heuristic about a year ago when working on something else and being surprised that none (I think) were actually ones we would have broken.)

nikolay commented 9 years ago

@oschaaf @jeffkaufman Not sure why I didn't get notified about your comments, guys. Sorry!

It says that this should be turned off by default on Nginx?

For new mod_pagespeed installs, this feature is turned on by default, for ngx_pagespeed it is off by default.

sligocki commented 9 years ago

The cost of rewriting a script which refs itself is much higher than the cost of not rewriting a script which doesn't, so having more False Positives than True Positives seems reasonable, but 0 True Positives sounds pretty surprising, what was the sample size? Before turning it off by default I think we need to do more analysis of effectiveness.

sligocki commented 9 years ago

@jeffkaufman: Do these uses still work once the script has been renamed? I'd expect them to break.

jeffkaufman commented 9 years ago

Those uses still work. They're scripts looking for their own DOM element, but then using that to add other DOM elements not to look for other files with parallel names.

nikolay commented 9 years ago

@sligocki I understand you point. I don't mind having to explicitly set the option myself, but I pointed out that the documentation says it's turned off by default and it doesn't seem to be.

jeffkaufman commented 9 years ago

@nikolay You're right; I'll fix the doc. It's actually always been on-by-default in ngx_pagespeed.

jeffkaufman commented 9 years ago

doc fix went live a few days ago