-
Notifications
You must be signed in to change notification settings - Fork 471
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
Add tests for optional chaining #2263
Comments
We already have an initial PR: |
I updated the OT with the checklist contents produced by @bcoe. |
@leobalter @DanielRosenwasser if we're happy with this check list as a start, I'll put some open source time in this weekend and work my way through a bunch of the cases; I love this sort of work. |
Only if you don't view it as an inconvenience - otherwise, I think it'd be awesome if you're up for it 😄 Leo will know more about the contribution process - ways that make it more approachable etc. |
This checklist is pretty comprehensive and I believe it's a great start. We can add interesting edgy cases along the way as we identify them. BTW: one edgy case that I discussed with my team this week, based on #2262: var stored = [];
var x = {
set y(v) { stored.push(`set y with ${v}`); }
get y() { stored.push('get y'); }
};
// dstr assignment
[ x?.y = 1 ] = [ 42 ];
assert.sameValue(stored.length, 1);
assert.sameValue(stored[0], 42); |
Im confused; why would optional chaining be permitted in a destructuring context? |
Checkout the ref issue. It’s valid to have a LHSExpr inside an
ArrayAssignment literal. Which I believe makes it consistent to have opt
chaining there.
…On Wed, Jul 31, 2019 at 8:29 PM Jordan Harband ***@***.***> wrote:
Im confused; why would optional chaining be permitted in a destructuring
context?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2263?email_source=notifications&email_token=AACJREKG6LCBGF3WKDSNP53QCIU5DA5CNFSM4IH7GK2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3I6EZI#issuecomment-517071461>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACJREMVFNJM5QEEX4HHMC3QCIU5DANCNFSM4IH7GK2A>
.
|
There is an explicit exception for this grammar, so |
it would be good to add tests for the edge-cases demonstrated here: v8/v8@ceb7bd5#diff-9b0c860cbf7e0482f43ad30124ee9f9bR92 Mainly:
|
Just a quick update, I have started work on the async tests for optional chaining (have just been fairly busy with a few other tasks). |
@DanielRosenwasser @leobalter @rkirsling, it feels like we could still improve coverage for Stage 4 in December. I saw the rollback of #2411, where should we ultimately end up in terms of generated tests vs., hand written tests, is it worth continuing to work down the check-lists we've written up ... alternatively, perhaps we're feeling that we've covered a healthy number of productions? |
@bcoe would you be able (time-wise) to update the coverage list or flag parts missing in the coverage? We can make a work effort after that or as you said, define coverage is reasonable enough. WDYT? |
@leobalter I'll make an effort to put some work into it this weekend, at least summarizing some of the productions we didn't quite hit yet. I definitely do think we're getting to the point where we have a healthy number of tests, but I think there are some areas we could flesh out more (as an example for and while statements). |
@leobalter @jridgewell @DanielRosenwasser, I'm feeling pretty good about our initial suite of tests (once we land #2429 which addresses some regressions @jridgewell brought up). |
Filing an issue to add tests for https://github.com/tc39/proposal-optional-chaining/
Statement List
UpdateExpression[Yield, Await]
AssignmentExpression[In, Yield, Await] (assignment-expression.js)
DestructuringAssignmentTarget[Yield, Await] (assignment-expression.js)
IterationStatement[Yield, Await, Return]: (iteration-statement.js)
ClassHeritage[Yield, Await]: (class-heritage.js)
Async Function
UpdateExpression[Yield, Await]
AssignmentExpression[In, Yield, Await] (async-assignment-expression.js)
DestructuringAssignmentTarget[Yield, Await] (async-assignment-expression.js)
IterationStatement[Yield, Await, Return]: (async-iteration-statement.js)
Iterator Function
UpdateExpression[Yield, Await]
AssignmentExpression[In, Yield, Await] (iterator-assignment-expression.js)
The text was updated successfully, but these errors were encountered: