webpack: detect if node-zopfli is installed before attemping to use it #4712
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcramer
reviewed
Dec 30, 2016
webpack.config.js
Outdated
Member
There was a problem hiding this comment.
fwiw we probably need this in getsentry and sentry-plugins, or we need to not use zopfli in those
Contributor
Author
There was a problem hiding this comment.
I'll replicate this to sentry-plugins too.
For some reason, it's not explicitly used for getsentry, so it doesn't need to go there, but we should probably add this snippet anyways to precompress there.
dcramer
reviewed
Dec 30, 2016
webpack.config.js
Outdated
Member
There was a problem hiding this comment.
why dont we just use the normal builtin gzip compression if zopfli isnt present
dcramer
approved these changes
Dec 30, 2016
Member
dcramer
left a comment
dcramer
left a comment
There was a problem hiding this comment.
lgtm if we do the gzip change
zopfli is not required by any means, especially for local development and CI. So if there's an issue installing this, we should just avoid attempting to compress rather than error all over the place. This keeps the error to just a simple warning instead so we're aware and can see this if something were to fail in a production build, but won't affect any other environment. Also, note that this is explicitly checking against paths rather than testing with a `require()` since with npm@2, the dependency is actually a sub-dependnecy of `compression-webpack-plugin` and can't be `require`d from outside that module. So it doesn't help to test that. And with npm@3, the module gets flatted to the top level, so we need to check both places.
mattrobenolt
added a commit
to getsentry/sentry-plugins
that referenced
this pull request
Dec 30, 2016
See getsentry/sentry#4712 for more context.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
zopfli is not required by any means, especially for local development
and CI. So if there's an issue installing this, we should just avoid
attempting to compress rather than error all over the place. This keeps
the error to just a simple warning instead so we're aware and can see
this if something were to fail in a production build, but won't affect
any other environment.
Also, note that this is explicitly checking against paths rather than
testing with a
require()since with npm@2, the dependency is actuallya sub-dependnecy of
compression-webpack-pluginand can't berequiredfrom outside that module. So it doesn't help to test that. And with
npm@3, the module gets flatted to the top level, so we need to check
both places.
I'll replicate this to
sentry-pluginstoo.