-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Optional Chaining Operator (Stage 1) #5813
Conversation
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.
- readme update with example and link to proposal
- does babel-types need changing?
- do we want some simple exec tests? (could be basis for test262 as well)
@hzoo: done. Please advise whether I have all the necessary LHS parents we need to guard. |
Nice job @jridgewell!
I'm ok with closing my PR. Link to parser PR babel/babylon#545. |
Done. Sorry about the duplicate PRs. I usually open one against the original PR (like the parser one), but this implementation was significantly different. |
const baz = obj?.foo?.bar?.baz(); // 42 | ||
|
||
const safe = obj?.qux?.baz(); // undefined | ||
const safe2 = obj?.foo.bar.qux?.(); // undefined |
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.
can we add an example like obj?.foo.bar.bazz()
that throws?
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.
|
||
const baz = new obj?.foo?.bar?.baz(); // baz instance | ||
|
||
const safe = new obj?.qux?.baz(); // undefined |
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 don't think this should return undefined (obv that depends on the proposal itself).
I think that if you want the new
not to throw when it gets a non-function, either the new
or the ()
must have the conditional operator attached to it.
In other words, new obj?.qux?.baz()
should be identical to const foo = obj?.qux?.baz; new foo()
.
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.
Moving to tc39/proposal-optional-chaining#2.
optionals.push(node); | ||
} | ||
|
||
objectPath = objectPath.get("object"); |
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.
for (let objectPath = path.get(key); objectPath.isMemberExpression(); objectPath = objectPath.get("object"))
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.
objectPath = objectPath.get("object"); | ||
} | ||
|
||
for (let i = optionals.length - 1; i >= 0; i--) { |
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.
for (const [i, node] of optionals.entries())
or
optionals.forEach((node, i) => {})
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.
Can't, I have to reverse iterate.
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.
Oh, right. .reverse()
could do the trick, but the i
would have to be subtracted from the length
🤔
if (atCall && t.isMemberExpression(chain)) { | ||
if (loose) { | ||
// To avoid a Function#call, we can instead re-grab the property from the context object. | ||
// `a.?b.?()` translates roughly to `_a.b != null && _a.b()` |
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.
That would invoke a getter twice though...
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.
Yup, that's why it's loose
. 😉
packages/babel-plugin-transform-optional-chaining/test/fixtures/execute/new.js
Show resolved
Hide resolved
@@ -416,7 +416,7 @@ defineType("LogicalExpression", { | |||
}); | |||
|
|||
defineType("MemberExpression", { | |||
builder: ["object", "property", "computed"], | |||
builder: ["object", "property", "computed", "optional"], |
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.
Does this need to be added to NewExpression
/ CallExpression
as well?
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.
Damn, missed that.
|
This way, it quickly returns from the conditions. The first nil will now exit, instead of checking every nil. This also allows conditionalChaining inside a container to still operate the container.
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 to me, thanks so much for the contribution! I'll work towards updating the TC39 proposal to cover some of the comments in this PR.
Does this mean that it has been added to the preset-stage-1? |
It's in there now. |
"babel-plugin-syntax-optional-chaining": "7.0.0-alpha.13" | ||
}, | ||
"keywords": [ | ||
"babel-plugin" |
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.
We can more keyword here:
- optional chaining
- null propagator
- elvis
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.
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.
Cyclops-Elvis operator
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.
Nice work guys 👍
Great job @jridgewell and @xtuc, thanks for the review @gisenberg!! |
This is awesome! Can this be released on NPM please? I was not able to install |
@pronebird see #5905 |
You want transform not syntax - https://github.com/babel/babel/releases/tag/v7.0.0-alpha.15, it's part of stage-1 too |
This is awesome! Can this support destructuring? |
Example: https://babeljs.io/repl/build/master#?babili=false&browsers=&build=&builtIns=false&code_lz=IYfgdARg3AUDqTAYzAE1vcFnnXBA2gOQREC6shEFmYAFAJRRA&debug=false&circleciRepo=&evaluate=false&lineWrap=false&presets=stage-0&prettier=false&targets=&version=7.0.0-beta.2
This is another implementation of Null Propagation Operators, competing with #5786. This one takes a different, top-down approach to the transform that avoids the
WeakSet
state. It also implements optionalCallExpression
s andNewExpression
s.This has a few todos:
manipulateOptions
stuff)CallExpression
s don't have acallee
NewExpression
s have acallee
that's aCallExpression
. That callee'scallee
andarguments
should be theNewExpression
scallee
andarguments
babel-generator
printer