bbc / Imager.js

Responsive images while we wait for srcset to finish cooking
Apache License 2.0
3.84k stars 224 forks source link

Minifying Imager with uglifyJS compression and mangle options #78

Closed nkp closed 10 years ago

nkp commented 10 years ago

I've noticed that in the past the dist build has not been compiled with the Mangle and Compress flags (-m and -c respectively).

I was wondering if this was a deliberate style choice, perhaps for debugging with the minified build.

Minifying with these options shows an almost 25% reduction in file size against the existing minified document.

$ uglifyjs Imager.js | wc -c
    6663

$ uglifyjs Imager.js -c -m | wc -c
    5014

Also, the minified build hasn't been rebuilt in 2 months.

thom4parisot commented 10 years ago

@nkp simply because at the time it was simply a Grunt task migrated to an npm task. Isn't the mangle possibly break in some browsers?

The minified version has not been rebuilt since the latest release I think, but that's also a good reminder to have a npm run release task to avoid forgetting tagging, packaging and publishing.

Thanks for your feedbacks, they are helpful :-)

nkp commented 10 years ago

jQuery 1.x has gone to great lengths to support all browsers and uses uglify-js' default mangle settings. Methods/fields exposed by the Imager object behave fine.

The only issues I've found with mangling the Imager.js source is the unit tests for the getPageOffsetGenerator in test/unit/core.js fail because they rely on source inspection. They could be modified slightly to pass by removing the reference to the window and document variable names.

E.g.

expect(generator.toString()).to.have.string('window.pageYOffset');

becomes

expect(generator.toString()).to.have.string('.pageYOffset');

Sounds like a good idea, it's nice to automate such things.

thom4parisot commented 10 years ago

Thanks @nkp, the next release will be even more compressed thanks to you :-)