-
-
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
Remove old optional chain features #6345
Conversation
jridgewell
commented
Sep 29, 2017
- Optional New is removed
- Put operations on an optional chain are removed
- Deletes on optional chain are removed
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6679/ |
/cc @claudepache |
For the deleted paths, what is the behaviour? Is there a syntax error triggered at some point? Another recent change was to change the scope of short circuiting. For example, |
These removals have to be updated in Babylon so tbey’re no longer valid. |
They are not removed in the official proposal, and I don’t expect them to be removed. |
I didn’t see it in the spec, where is it handled at? |
There is no change needed in the current spec around delete in order to make it work. |
Also, some invalid constructs that should be checked: The new operator and tagged template literals can no longer be used after optional chaining, e.g.: Attempt to assign to an optional chain (e.g. |
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.
A few issues with this patch and the existing optional chaining transform:
- Seems like this expands to
== null
, but the specified semantics are=== undefined || === null
. The difference is in the semantics of thedocument.all
object, which passes the test you wrote but not the test in the spec. (== null
is fine for loose mode though). - Should line 13 be deleted as part of getting rid of optional chaining new? Ditto the
isNewExpression
on line 33. - I see a bunch of tests deleted; I'd prefer to see them test for the newly intended behavior (and the previous version would fail against the new tests)
- Rather than using the
.call
method, maybe better to useFunction.prototype.call.call
(or maybe that doesn't help, I dunno) - When finding
?.
used in places which are disallowed, such asx?.y = z
, this should be a SyntaxError because IsValidSimpleAssignmentTarget is false onx?.y
. Is this implemented? Ditto fornew x?.(y)
(though this becomes allowed again, without any chaining semantics, if you add parens likenew (x?.(y))
. - I disagree with @claudepache about optional chain delete being good to keep in; we're discussing it at Ban optional chaining delete (and anything else that may be missing?) tc39/proposal-optional-chaining#40 . As long as the spec has optional chaining delete in it, IMO Babel should continue to support it. As things are now, I'd guess that
delete x?.y
might just consistently evaluate totrue
without deleting anything; if it is removed, this should become a syntax error.
|
Thanks for clarifying what the specified behavior is. My comment was referring to the Babel source. |
e09704a
to
eaaec82
Compare
This has been rebased. To answer a few of @littledan's points:
This was fixed on master, and is in the rebase.
This would require a large redo of the babylon code to accomplish. I've added checks for
Can someone walk me through how this is handled by the spec? Without it being explicitly called out, I don't see how the current text allows it. |
I think we should land with what's here and make a new PR for new fixes then? |
Sounds like some good follow-on work.
|