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

Remove minification from package #3

Open
jescalan opened this issue Sep 5, 2016 · 8 comments
Open

Remove minification from package #3

jescalan opened this issue Sep 5, 2016 · 8 comments

Comments

@jescalan
Copy link
Member

jescalan commented Sep 5, 2016

While it's a convenient feature, its almost just as easy to simply add minification manually when necessary, and the minify package is nearly 10x the size of all the other dependencies of this package, which seems a little weighty to include as a disabled-by-default optional.

@smuemd
Copy link

smuemd commented Jan 13, 2017

Also there seems to be a bug w/ minify:
when I was running this via -e production, all the automatically set IDs (pageId: pageId(ctx)) vanished from the html output.
Have you experienced this too?

-S

@tmpfs
Copy link

tmpfs commented Jan 16, 2017

@smuemd, I am experiencing the same problem with vanishing page identifiers and minification too.

@jescalan do you have any recommendations for how we should consume the HTML minifier logic once it is removed from reshape-standard, I am happy to toggle the minify flag and do it now because this breaks my production deployment :(

@jescalan
Copy link
Member Author

For now, I'd say don't run minify. The savings from it are convenience-level, like a couple bytes. I'll look at this and patch it as soon as I can. You are also welcome to dig in and submit a patch to fix it if you want to beat me in speed!

@smuemd
Copy link

smuemd commented Jan 17, 2017

Error could be in anywhere, since it fails silently. @jescalan any guess where to start digging?
Mifify? Or maybe spike-page-id itself? Or somewhere else entirely?

@jescalan
Copy link
Member Author

I'd probably start in the minify plugin, disabling features one by one until I can find the one that is causing it to be stripped. Before that of course checking to make sure it comes in to minify in the first place!

@liitfr
Copy link
Contributor

liitfr commented Jan 30, 2017

So I've just tried to use minification option.
Here are two other known issues :

  1. I couldn't use Minify if there is a SVG tag inside my SGR
  2. I couldn't use it if there's some inline JS that use a locals variable.

Here is an exemple of second issue :

      script.
        window.ga=function(){ga.q.push(arguments)};ga.q=[];ga.l=+new Date;
        ga('create','{{ config.googleSiteId }}','auto');ga('send','pageview')

{{ config.googleSiteId }} makes compilation fail.

Do you want me to create two more issues or keep it here, just as a reminder ?
Let me know :)
Mat

@jescalan
Copy link
Member Author

Thanks for catching these! Yeah we can just keep it here, when I have a minute I'll try to tackle all these issues. In the meantime, anyone else is very welcome to take a stab!

@jescalan
Copy link
Member Author

Hey for anyone still watching here, the minify issues was closed a couple months ago and it's now working perfectly 😁

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

4 participants