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
refactor(parser): remove refNeedsArrowPos #13419
refactor(parser): remove refNeedsArrowPos #13419
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46979/ |
The CI failure seems to be unrelated. |
I received the same message for my previous PR. |
I am wondering if we could remove all the babel/packages/babel-parser/src/parser/expression.js Lines 346 to 348 in a0369fd
it is not engaging with the parser. The
We could try removing it and see if it breaks any test. |
Hey 👋 @JLHwung ^^ It breaks a lot of tests (20 at least) and as you predicted it's on flow an typescript tests. For example in this code:
Maybe could we pass a boolean instead of WDYT ? |
Ping @JLHwung ^^ |
@tony-go It seems to me
refNeedsArrowPos .
|
Hi 👋 @JLHwung Regarding the issue #13419 I pushed a test branch (which contains the move of Where I’m trying to figured out why test fails :/ Even with the callstack debugger I didn’t understand why en expression like const f = (x?) => {} throw there. (modifié) |
Update: I continue to dig with the help of @JLHwung ^^ We merge few things but it still remains two tests which fails. I'm actively working on it ^^ Sorry for the wait. |
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 6d9321e:
|
Let me fix linter :/ |
this.parseExprListItem( | ||
false, | ||
refExpressionErrors, | ||
{ start: 0 }, |
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.
Q: refNeedsArrowPos
is reset here. Do we need to reset refExpressionErrors.optionalParameters
to -1
? Maybe worth a test covering this.
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 like how there are more deleted lines than added lines 😄
Did all corrections. Just left this [comment](#13419 (comment):
As I did found the test relevant test case for now :/ |
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.
The PR generally looks good to me! Left some nit comments on extra checking.
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.
Thanks!
Thanks you for you time. You (and @nicolo-ribaudo) provide me an excellent OSS experience ^^ |
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'm surprised that we need these changed in code that looks unrelated 🤔
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.
🎉
Hey @JLHwung 👋
Following your comment on my previous PR, I open this draft PR. Let me know If I'm wrong or If I misunderstand something.
🚀