fix: add support for terser 5 under node 12 and higher #2558
Conversation
This file contains 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
Aghassi
force-pushed
the
fix/support-terser-5
branch
from
130a0c2 to
58a30c3
Compare
Aghassi
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
Aghassi
force-pushed
the
fix/support-terser-5
branch
from
58a30c3 to
f85bb87
Compare
Aghassi
force-pushed
the
fix/support-terser-5
branch
from
f85bb87 to
94f61ad
Compare
alexeagle
reviewed
Apr 6, 2021
| 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') { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
the comment is confusing, is this branch specific to terser 5? or just node 12 and up?
alexeagle
approved these changes
Apr 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
On Node 14 with Terser 5, Terser binary will fail to to be found as
require.resolvewill error out. See terser/terser#971Issue Number: N/A
What is the new behavior?
We will fall through and attempt to do different resolve logic based on the version of Node
Does this PR introduce a breaking change?
Other information