Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add support for terser 5 under node 12 and higher #2558

Merged
merged 1 commit into from
Apr 18, 2021
Merged

fix: add support for terser 5 under node 12 and higher #2558

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions packages/terser/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,30 @@ function main() {
// Allow for user to override terser binary for testing.
let terserBinary = process.env.TERSER_BINARY;
try {
// If necessary, get the new `terser` binary, added for >=4.3.0
terserBinary = terserBinary || require.resolve('terser/bin/terser');
// Node 12 and above will respect exports field in package.json, Terser 5 added these
// `process.version` returns vx.x.x, here we strip the `v` and get the major number
if (process.version.split('.')[0].replace('v', '') <= '10') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look around the repo for other js code that does semver parse, there's probably a more reliable way. I'm worried that this is doing string comparison rather than numbers

// If necessary, get the new `terser` binary, added for >=4.3.0 <5
terserBinary = terserBinary || require.resolve('terser/bin/terser');
} else {
// Terser 5 with Node 12 or higher breaks compatiability with how Rules NodeJS supports
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is confusing, is this branch specific to terser 5? or just node 12 and up?

// getting the terser bin. An issue has been filed https://github.com/terser/terser/pull/971
// This is a temporary work around to allow us to proceed without downgrading
// or changing versions
// This gets the full path that to node_modules
// Then we hardcode the path to the bin directory since that's what we are looking for
// This will return a path like
// /private/var/tmp/_bazel_david.aghassi/a7bb26caa05a7d74fdb20e24a0f896f3/external/npm/_/node_modules
switch (os.platform()) {
case 'win32':
terserBinary = terserBinary ||
`${require.resolve('terser').split('\\terser\\')[0]}\\terser\\bin\\terser`;
break;
default:
terserBinary =
terserBinary || `${require.resolve('terser').split('/terser/')[0]}/terser/bin/terser`;
}
}
} catch (e) {
try {
// If necessary, get the old `uglifyjs` binary from <4.3.0
Expand Down