Gracefully handle missing uglify-js dependency#1394
Conversation
nknapp
force-pushed
the
graceful-missing-uglify
branch
2 times, most recently
from
1f7eb34 to
d6b70f8
Compare
|
I'll merge this request to 4.x and master on Monday evening (Europe) if nobody objects. Please let me know if you need it earlier. |
| if (err.code !== 'MODULE_NOT_FOUND') { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
@nknapp it might be easier to move the require() call into the if (opts.min) { condition block below. that way you will only try to import the dependency if someone is actually trying to use the min option and you won't need the try-catch at all.
|
@nknapp thanks a lot for looking into this. we've addressed this issue on the Ember CLI end already too so this is not time critical anymore, but probably still good to fix it :) |
|
I thought it would be better for the readability of the code if the require was at the beginning. But you are probably right. There is no need to load uglify when the user does not want to minify in the first place. |
|
Have added another commit and I will squash them before merging. Thanks for reviewing 😏 |
| }); | ||
| } else { | ||
| console.error('Code minimization is disabled due to missing uglify-js dependency'); | ||
| } |
There was a problem hiding this comment.
why not just use try/catch here directly instead of if/else and the function above?
There was a problem hiding this comment.
I want to use require.resolve to detect if uglify exists, beause it wont be irritated by other missing-module-errors (caused by dependencies of uglify).
I could put the .minify-call into the try-catch-block, but that would be something like
A function seems to be the best way to create a try-catch construct
let exists;
try {
require.resolve('uglify-js');
exists = true
} catch (e) {
if (e.code !== 'MODULE_NOT_FOUND') {
throw e;
}
exists = false
}
if (exists) {
require('uglify-js').minify(...)
}This seems to be more complicated and I don't particular like the use of the exists variable in this code. Creating a function seems to be more readable.
But I think I will still make a change.
closes #1391 uglify-js is an optional dependency and should be treated as such. This commit gracefully handles MODULE_NOT_FOUND errors while loading uglify. - Check for existing uglify-js (and load uglify-js) only if minification was activated - Use "require.resolve" to check if uglify exists. Otherwise, a missing dependency of uglify-js would cause the same behavior as missing uglify-js. (Only a warning, no error) - The code to load and run uglify is put into a single for readability purposes - Tests use a mockup Module._resolveFilename to simulate the missing module. This function is used by both "require" and "require.resolve", so both are mocked equally.
nknapp
force-pushed
the
graceful-missing-uglify
branch
from
2a46f3f to
2bbb2db
Compare
2 participants
closes #1391
uglify-js is an optional dependency and should be treated as such.
This commit gracefully handles MODULE_NOT_FOUND errors while loading
uglify.