Skip to content
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

[BugFix] : OptionalChaining Bug fixes #7288

Merged
merged 6 commits into from Feb 8, 2018

Conversation

@nveenjain
Copy link
Contributor

nveenjain commented Jan 29, 2018

Q                       A
Fixed Issues? Fixes #7256
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

Created new node types on babel-types and babylon. OptionalMemberExpressions and OptionalCallExpressions, which while transforming are converted to Normal MemberExpression and CallExpressions (to satisfy transformed AST). Added some tests to babylon and transforms. This will be a breaking change

  1. for those plugins who are using MemberExpression as of now as they'll have to change visitor's entry point
  2. for those plugin who convert to OptionalChain right now, as after this commit, MemberExpression will no longer have optional property.

Suggestions/edits are welcome.

@nveenjain nveenjain changed the title [WIP] OptionalChaining Bug fixes [BugFix] : OptionalChaining Bug fixes Jan 30, 2018
@nveenjain nveenjain mentioned this pull request Jan 30, 2018
4 of 4 tasks complete
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jan 30, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6832/

Copy link
Member

Qantas94Heavy left a comment

Just had a very quick glance, so not a thorough review. Mostly LGTM.

@@ -454,7 +454,7 @@ export default class ExpressionParser extends LValParser {
);
} else if (this.match(tt.questionDot)) {
this.expectPlugin("optionalChaining");

state.optionalChainMember = true;

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Jan 31, 2018

Member

Not sure if this is relevant to this case, but is there any issue with state possibly being reused and optionalChainMember being true when it shouldn't?

This comment has been minimized.

Copy link
@nveenjain

nveenjain Jan 31, 2018

Author Contributor

optinalChainMember is true only after encountering ?. so i don't think there might be any issue. Once we encounter ?. then we are sure that the rest of chain will be optionalChain Member with optional value = false ( meaning that the node is in optionalChain but it is not optional(root of optional chain) ). When we encounter ')' like in (a?.b).c then the chain is no longer optional and so state doesn't have optionalChainMember property for (a?.b) object. Should i also add (a?.b).c in test cases to make it more clear or can we make it a good first issue for first timers looking for contribution?

This comment has been minimized.

Copy link
@nveenjain

nveenjain Feb 2, 2018

Author Contributor

@Qantas94Heavy , i added additional test cases, please take a look. Maybe that clear the confusion or is there still some doubt?

Copy link
Member

jridgewell left a comment

We need assertions for:

obj?.fn`template` // parse error

super?.obj // parse error
if (!path.node.optional) {
path.node.type =

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 7, 2018

Member

We can simplify the findReplacementPath by not changing the node type here.

@@ -454,7 +454,7 @@ export default class ExpressionParser extends LValParser {
);
} else if (this.match(tt.questionDot)) {
this.expectPlugin("optionalChaining");

state.optionalChainMember = true;
if (noCalls && this.lookahead().type == tt.parenL) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 7, 2018

Member

I think we might need to throw here to prevent new fn?.() and new obj?.fn()

new B?.C?.(a, b)

new B?.C
new C?.b.d()

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 7, 2018

Member

We also need a test for new C?.()

@@ -0,0 +1 @@
new a?.();

This comment has been minimized.

Copy link
@nveenjain

nveenjain Feb 7, 2018

Author Contributor

@jridgewell , is this test case OK?

This comment has been minimized.

Copy link
@jridgewell
@nveenjain

This comment has been minimized.

Copy link
Contributor Author

nveenjain commented Feb 7, 2018

Also @jridgewell , since issue #7256 didn't have super/tagged template literal as it's key points, i've not implemented them in this PR. Do you mind if I open another issue stating them (super?.key), and then make a PR??

@@ -128,7 +122,8 @@ export default function(api, options) {
inherits: syntaxOptionalChaining,

visitor: {
"MemberExpression|CallExpression"(path) {
"OptionalCallExpression|OptionalMemberExpression"(path) {
debugger;//eslint-disable-line

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 7, 2018

Member

Remove.

This comment has been minimized.

Copy link
@nveenjain

nveenjain Feb 8, 2018

Author Contributor

Sorry for that 😅 , test cases were passing and i completely forgot that lived here.

@@ -0,0 +1 @@
new a?.();

This comment has been minimized.

Copy link
@jridgewell
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Feb 7, 2018

Do you mind if I open another issue stating them (super?.key), and then make a PR??

Works for me.

@jridgewell jridgewell merged commit a3ad518 into babel:master Feb 8, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.05% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nveenjain nveenjain deleted the nveenjain:fix/optionalPlugin branch Feb 8, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
* Added optionalExpression types to babylon and babel-types

* OptionalChain transforms bug fix

* Added OptionalExpressions to babel-generator. Fixed OptionalChain Bugs

* Removed 'optionalChain' from newExpression and added test cases

* Added test cases for optionalChain

* Update index.js
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.