Add gfx-zopfli #449
Add gfx-zopfli #449
Conversation
swojit
force-pushed
the
sw/add-gfx-zopfli
branch
2 times, most recently
from
e302bbd
to
c6a06a2
Compare
There was a problem hiding this comment.
Nice work! Just for future reference, can you add a bit more description to this pull request? I would like to see what the motivation of using this library is, some description of what it does to the behavior and performance profile of a Fusion.js application, and what kind of testing you performed with this.
|
I will definitely keep that in mind. I avoided adding more description because I felt there was nothing more to add after Ryan's description in the referenced issue. |
|
Do we have tests for these compression plugins to make sure that it is actually compressing the files when requested? |
|
Also, please edit the description with the information you provided in your comment so that we can use it as release notes. Thanks! |
|
@mlmorg I am not sure if we have tests for the compression plugins. Let me look that up. |
build/compiler.js
Outdated
| @@ -17,6 +17,7 @@ const ProgressBarPlugin = require('progress-bar-webpack-plugin'); | |||
| const webpackDevMiddleware = require('../lib/simple-webpack-dev-middleware'); | |||
| const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin'); | |||
| const { | |||
| gfxWebpackPlugin, | |||
There was a problem hiding this comment.
I think zopfliWebpackPlugin would be a more apt name since it indicates the purpose. It's hard to tell what gfxWebpackPlugin is based on the name.
swojit
force-pushed
the
sw/add-gfx-zopfli
branch
from
950da55
to
8bbc22d
Compare
lib/compression/index.js
Outdated
| @@ -9,6 +9,7 @@ | |||
| /* eslint-env node */ | |||
|
|
|||
| module.exports = { | |||
| zopfliWebpackPlugin: require('./gfx-webpack-plugin'), | |||
There was a problem hiding this comment.
gfx is the github handle for the wasm binding lib author. This file should probably be called zopfli-webpack-plugin
test/cli/build.js
Outdated
| files.filter(file => getFileExtension(file) === 'br'), | ||
| 'brotli works' | ||
| ); | ||
| t.ok(files.filter(file => getFileExtension(file) === 'svg'), 'svg works'); |
There was a problem hiding this comment.
Is this testing SVGO? If so maybe check that the file contents don't have consecutive spaces
There was a problem hiding this comment.
Or I can just compare file sizes?
There was a problem hiding this comment.
yeah that works too. As long as it does more than just check for file existence, since IIRC, the file gets created regardless of whether the compression plugin is there or not
There was a problem hiding this comment.
Ah, that makes sense.
test/cli/build.js
Outdated
| path.resolve(dir, '.fusion/dist/production/client/'), | ||
| (err, files) => { | ||
| if (err) throw err; | ||
| t.ok(files.filter(file => getFileExtension(file) === 'gz'), 'gzip works'); |
There was a problem hiding this comment.
Maybe just use path.extname here
swojit
force-pushed
the
sw/add-gfx-zopfli
branch
from
8bbc22d
to
49a17fe
Compare
Fixes #111
After replacing node-nozopfli for its installation issues, Ryan suggested using @gfx/zopfli because it is a fast and universal alternative to the native library.
The behaviour doesn't change at all.
Performance for compression will be slightly slower for gzip compression but given node-zopfli's issues, it's not a dealbreaker.