-
-
Notifications
You must be signed in to change notification settings - Fork 222
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.
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
Remove folding Array.prototype.concat (fix #577) #578
Conversation
/cc @j-f1 |
Codecov Report
@@ Coverage Diff @@
## master #578 +/- ##
==========================================
+ Coverage 82.86% 83.02% +0.16%
==========================================
Files 44 44
Lines 2900 2928 +28
Branches 1012 1022 +10
==========================================
+ Hits 2403 2431 +28
Misses 303 303
Partials 194 194
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple comments:
@@ -107,9 +107,6 @@ describe("constant-folding-plugin", () => { | |||
it("should handle Array methods on array literals", () => { | |||
const source = unpad( | |||
` | |||
[1, 2, 3].concat([4, 5, 6]); | |||
[a, b, c].concat([d, e], f, g, [h]); | |||
[1, 2, 3]["concat"]([4, 5, 6]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to test this with push
since you’re removing it:
[1, 2, 3]["push"]([4, 5, 6])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
return t.arrayExpression( | ||
this.elements.concat( | ||
...args.map(arg => { | ||
if (t.isArrayExpression(arg)) return arg.elements; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to check if any of these are variables instead of removing this entirely? For example, [a, b, c].concat([1, 2, 3])
is safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really common to use constants here in concat ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.