[ts] Check if param is assignable when parsing arrow return type#13581
[ts] Check if param is assignable when parsing arrow return type#13581nicolo-ribaudo merged 3 commits intobabel:mainfrom
Conversation
| return node.properties.every((prop, i) => { | ||
| return ( | ||
| prop.type !== "ObjectMethod" && | ||
| (i === last || prop.type !== "SpreadElement") && |
There was a problem hiding this comment.
The bug that caused the regression was that this was using === "SpreadElement" instead of !==.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Why use 'every' ? It's slow vs a native for-loop
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47484/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 24c017a:
|
|
The failure is a regression. |
|
|
||
| NOTE: There is a corresponding "isAssignable" method in flow.js. | ||
| NOTE: There is a corresponding "isAssignable" method. | ||
| When this one is updated, please check if also that one needs to be updated. |
There was a problem hiding this comment.
This note can be removed then since it is moved to base parser.
There was a problem hiding this comment.
I actually like this comment: even if they are in the same file, it's still easy to miss isAssignable when updating toAssignable.
| case "ObjectPattern": | ||
| case "ArrayPattern": | ||
| case "AssignmentPattern": | ||
| case "RestElement": |
There was a problem hiding this comment.
@nicolo-ribaudo What about a lookup table? This switch is slow because of 1) string comparison 2) many cases 3) hard to optimize in v8
There was a problem hiding this comment.
I don't think this switch relies on string comparisons: these strings are always constants in our code so V8 will atomize them and just compare them as pointers (but @JLHwung knows more than me about this)
There was a problem hiding this comment.
v8 will convert this switch to a lookup, so it's faster if you do it.
|
Merged, but performance not improved. What's the reason? |
|
That change doesn't have an observable impact on the normal |
|
Thank you guys :-) Then this release will land into |
|
It is fixed in |
This is the third attempt at fixing #11038:
SyntaxErroromitted from function params destructuring #12195 (test for the regression added by test: add test case for babel-parser: fixtures/typescript/arrow-function/destructuring-with-annotation-newline #12203)