standard icon indicating copy to clipboard operation
standard copied to clipboard

Remove minification from package

Open jescalan opened this issue 8 years ago • 8 comments

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.

jescalan avatar Sep 05 '16 22:09 jescalan

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

smuemd avatar Jan 13 '17 22:01 smuemd

@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 :(

tmpfs avatar Jan 16 '17 18:01 tmpfs

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!

jescalan avatar Jan 16 '17 21:01 jescalan

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?

smuemd avatar Jan 17 '17 22:01 smuemd

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!

jescalan avatar Jan 17 '17 22:01 jescalan

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

liitfr avatar Jan 30 '17 19:01 liitfr

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 avatar Jan 30 '17 20:01 jescalan

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

jescalan avatar Apr 14 '17 04:04 jescalan