Make JSqueeze filter work with 1.x and 2.x (namespaced) #698
Conversation
AdamWill
force-pushed
the
jsqueeze2
branch
3 times, most recently
from
8fd663a
to
4854406
Compare
|
(if anyone's watching this, please play Yakity Sax at this point) |
| // Upstream default for special var is false in 2.x | ||
| if (class_exists('\\Patchwork\\JSqueeze')) { | ||
| $this->classname = '\\Patchwork\\JSqueeze'; | ||
| $this->specialVarRx = false; |
There was a problem hiding this comment.
You should provide the same default I believe if you want 1 vs 2 to be transparent \Patchwork\JSqueeze::SPECIAL_VAR_PACKER in 2.0
There was a problem hiding this comment.
I wasn't really sure whether to do that or respect the upstream default for the current jsqueeze. As another idea, maybe make it default false in both cases, as experience seems to have suggested that's the best choice (which is why you changed it upstream in 2.x, right?)
There was a problem hiding this comment.
false by default in 1 and 2 would be fine for me. As you noticed, the option allows increasing the compression ratio but lets have a safer default
|
@nicolas-grekas how about this version? the global renaming is disabled by default for 1.x and 2.x but you can do edit: looks like it works. Well, it doesn't seem to be safe to use with OC - if I enable the global renaming I get buggy OC behaviour - but it does what it's supposed to do, I think. |
Also disables the global var renaming behaviour by default for both 1.x and 2.x (this seems like the safest choice) but allows setSpecialVarRx(true) to enable it with the default regex.
|
Thank you @AdamWill. |
…dam Williamson) This PR was merged into the 1.2-dev branch. Discussion ---------- Make JSqueeze filter work with 1.x and 2.x (namespaced) JSqueeze 2.x just came out, and it's now namespaced. This should let the filter work with both 1.x and 2.x. Obviously it'd also be simple to just make it work with 2.x only if you wanted to go that way. I thought about adding some helper bits to make it easier to use upstream's default special var regex on 2.x, but thought I'd just keep it simple for now. We could do something like have `setSpecialVarRx` accept 'true', and have that set the value to `JSqueeze::SPECIAL_VAR_PACKER` on 2.x. @nicolas-grekas wdyt? It'd be good to have this on the stable branches too. Commits ------- 0182f4e Make JSqueeze filter work with 1.x and 2.x (namespaced)
This disables the special var renaming feature of Jsqueeze by default, matching the change done in kriswallsmith/assetic#698
|
@stof thank you! would it be possible to get it on the 1.2 branch too? |
The JSMin minifier is non-free. JShrink is free, it's also a currently-maintained project following good development practices (though we may wish to switch to JSqueeze soon for performance reasons; waiting on kriswallsmith/assetic#698 reaching upstream Assetic 1.2 for that). This goes along with a 3rdparty commit that drops mrclay/minify and adds JShrink.
JSqueeze 2.x just came out, and it's now namespaced. This should let the filter work with both 1.x and 2.x. Obviously it'd also be simple to just make it work with 2.x only if you wanted to go that way.
I thought about adding some helper bits to make it easier to use upstream's default special var regex on 2.x, but thought I'd just keep it simple for now. We could do something like have
setSpecialVarRxaccept 'true', and have that set the value toJSqueeze::SPECIAL_VAR_PACKERon 2.x.@nicolas-grekas wdyt?
It'd be good to have this on the stable branches too.