reshape/standard

Remove minification from package

Opened this issue · 8 comments

jescalan commented

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 commented

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 commented

@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 commented

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 commented

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 commented

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 commented

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 commented

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 commented

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