Skip to content

Minify inline #823

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Minify inline #823

wants to merge 4 commits into from

Conversation

javdl
Copy link

@javdl javdl commented Feb 2, 2016

Minify styles & scripts which are placed inline. For example the Google Analytics code will be minified.

comments: true
}
},
jsSelector: 'script[type!="text/x-handlebars-template"]',
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, we don't use handlebars in the project. We shouldn't add exclusions to things not used in the repo.

@surma
Copy link
Contributor

surma commented Feb 2, 2016

@Joostvanderlaan Thanks for the effort. I definitely agree that we should minify inline stuff as well. Both @Garbee and myself commented on a few things that need addressing before we merge, tho :)

@javdl
Copy link
Author

javdl commented Feb 3, 2016

@surma I just pushed the changes you and @Garbee suggested, integrated them to the best of my knowledge.

comments: false
}
},
jsSelector: 'script[type!="text/x-handlebars-template"]',
Copy link
Contributor

Choose a reason for hiding this comment

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

This either needs to be removed or we need to do something custom like the cssSelector one. We should not have ignores for things our repo doesn't use.

@Garbee
Copy link
Contributor

Garbee commented Feb 3, 2016

Still one of the old issues exists.

Thinking about this though, there is one problem doing this. Content Security Policy allows you to pre-hash inline scripts and styles to validate they are safe to run. Auto-minifying is going to change the hash value for developers. This increases the barrier to integrating CSP for developers to realize what is going on.

We should be very mindful adding this in. Is it really providing a major enough benefit to make developers adopting CSP possibly be very confused as to why things aren't working?

@javdl
Copy link
Author

javdl commented Feb 3, 2016

@Garbee CSP suggests outlining all scripts (styles are more or less OK). Only when it's really not possible to outline scripts you may inline them with a hash. But since the GA script has the variable UA-XXXXX-X in there, I don't see hashing happening for anyone using WSK unless some Gulp task is included to create that hash for for them.

<script nonce=EDNnf03nceIOfn39fn3e9h3sdfa>
  //Some inline code I cant remove yet, but need to asap.
</script>

@Garbee
Copy link
Contributor

Garbee commented Feb 3, 2016

The actual specification shows no preference to suggesting external scripts. However, since the nonce attribute is required on them, we can do an easy filter out of those scripts in the config. That is what we should do by default to help avoid this problem.

@surma
Copy link
Contributor

surma commented Feb 3, 2016

My 2¢: Hashing for CSP should be done after minification in the build process. To much data can be saved to justify skipping minification on inline styles and scripts.

@Garbee
Copy link
Contributor

Garbee commented Feb 3, 2016

Is there a gulp plugin that can reasonably do that?

I believe the Pagespeed mod for apache and nginx can, but it is a beast of its own in terms of page control.

@javdl
Copy link
Author

javdl commented Feb 5, 2016

FYI, my PR for data-do-not-minify is merged into gulp-minify-inline

@surma
Copy link
Contributor

surma commented Feb 8, 2016

cc @gauntface @addyosmani

@Garbee Garbee mentioned this pull request Feb 22, 2016
@gauntface
Copy link

This gets a +1 from me.

If CSP becomes an issue for someone, I'd expect them to be aware of ahead of time and address with additional tooling as needed (or simply remove the steps in this task).

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

Successfully merging this pull request may close these issues.

5 participants