Conversation
|
Any measurements? |
| return false; | ||
| } | ||
| const { expression, arguments } = callExpression as CallExpression; | ||
| const { expression, arguments: args } = callExpression as CallExpression; |
There was a problem hiding this comment.
Would it be better to just rename property in CallExpression? Also would this change should be applied to newExpression as well?
There was a problem hiding this comment.
That would be a breaking change to API consumers.
|
@RyanCavanaugh there was a spike in the build-to-build performance tests associated with this change. Whether this was the direct cause I haven't yet determined. Regardless of the performance metrics, we should never shadow |
|
The PR that added this code was #14984 on April 3rd. The performance tester showed emit time spike after that commit, but this function is called by the binder and checker. In the time since, emit time has gone back down. |
|
@Andy-MS the initial So any assignment to That said, I ran a javascript project through tsc with hydrogen tracing and didn't see any specific deoptimizations in this function. I still think its worth the change. |
|
Ah, forgot we're compiling to ES5. |
This may be affecting performance? CC @rbuckton