fix(terser): call terser binary instead of uglifyjs #1360
Conversation
josephperrott
force-pushed
the
terser-update
branch
from
2eb15ce to
54b6103
Compare
|
josephperrott
requested review from
alexeagle and
Toxicable
and removed request for
alexeagle
josephperrott
added
dependencies
josephperrott
requested review from
alexeagle,
gregmagolan and
soldair
as code owners
|
Thanks for picking this up @josephperrott . That warning message was very spammy. |
|
@Toxicable the I am unsure of what changes occured between the versions, but it seems that they already are accounted for in testing/usage |
packages/terser/src/index.js
Outdated
| @@ -160,7 +160,7 @@ function main() { | |||
| const residual = argv.slice(outputArgIndex + 2); | |||
|
|
|||
| // user override for the terser binary location. used in testing. | |||
| const terserBinary = process.env.TERSER_BINARY || require.resolve('terser/bin/uglifyjs') | |||
| const terserBinary = process.env.TERSER_BINARY || require.resolve('terser/bin/terser') | |||
There was a problem hiding this comment.
Looks like terser/bin/terser was only added in terser 4.3.0 so this would break users that are on terser > 4.0 < 4.3.
We could either change the peer dep to be >= 4.3 but I think the better solution is to change the logic to this:
process.env.TERSER_BINARY || require.resolve('terser/bin/terser') || require.resolve('terser/bin/uglify')
in a try/catch (as require.resolve will throw if it doesn't find terser/bin/terser) and add a comment that terser/bin/terser is only available for >= 4.3.0
packages/terser/src/index.js
Outdated
| @@ -171,7 +171,7 @@ function main() { | |||
| if (!inputs.find(isDirectory) && inputs.length) { | |||
| // Inputs were only files | |||
| // Just use terser CLI exactly as it works outside bazel | |||
| require(terserBinary || 'terser/bin/uglifyjs'); | |||
| require(terserBinary || 'terser/bin/terser'); | |||
There was a problem hiding this comment.
@alexeagle This should be fine as just require(terserBinary) right given that terserBinary is already resolved above?
There was a problem hiding this comment.
I don't see why it needs another case here, maybe it's overabundance of caution. @soldair wrote that bit I think
No description provided.