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

Add optional function call to optional chaining #1585

Merged
merged 1 commit into from Feb 17, 2020

Conversation

@ilogico
Copy link
Contributor

ilogico commented Feb 13, 2020

I noticed the syntax for the optional function call wasn't being tested f?.().
I also slightly changed the test for optional method call to better guarantee this is correct.

@ljharb
ljharb approved these changes Feb 13, 2020
Copy link
Collaborator

ljharb left a comment

Pending confirmation that all the results here have been manually verified.

@ilogico

This comment has been minimized.

Copy link
Contributor Author

ilogico commented Feb 14, 2020

I didn't confirm the results manually, I assumed they were the same as the rest.
I did confirm them on the latest Firefox, Chrome and TypeScript.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 15, 2020

In that case, please remove all the results you didn't confirm manually :-) future PRs can add the results as they're tested.

@ilogico ilogico force-pushed the ilogico:optional-function-call branch from 693e597 to ea35a85 Feb 17, 2020
@ilogico

This comment has been minimized.

Copy link
Contributor Author

ilogico commented Feb 17, 2020

I removed the results I couldn't confirmed.
I kept the false results which are guaranteed to be false (ie11 and firefox < 73)

Copy link
Collaborator

chicoxyzzy left a comment

Both optional method call and optional function call tests return true in Safari 13.1 Beta and Firefox 74 Beta. Though we can add these results in the following PR.

chrome77: false,
chrome78: {val: 'flagged', note_id: "chrome-optional-chaining"},

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Feb 17, 2020

Collaborator

I think that this result is still valid, we should re-check it

var bar = null;
return foo?.baz() === 42 && bar?.baz() === undefined;
*/},
res : {
babel7corejs2: true,

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Feb 17, 2020

Collaborator

This result is still valid

Copy link
Collaborator

chicoxyzzy left a comment

Thinking again, all removed test results from optional method call tests should be added again. I've rechecked them all manually.

@ilogico

This comment has been minimized.

Copy link
Contributor Author

ilogico commented Feb 17, 2020

Should I add them again?

@chicoxyzzy

This comment has been minimized.

Copy link
Collaborator

chicoxyzzy commented Feb 17, 2020

@ilogico yes please

@ilogico ilogico force-pushed the ilogico:optional-function-call branch from ea35a85 to f92986e Feb 17, 2020
@ilogico

This comment has been minimized.

Copy link
Contributor Author

ilogico commented Feb 17, 2020

Done

@chicoxyzzy

This comment has been minimized.

Copy link
Collaborator

chicoxyzzy commented Feb 17, 2020

Oh, sorry, I mentioned only optional method call test results, not optional function call. But that's ok, I'll check them too. Thanks!

Copy link
Collaborator

chicoxyzzy left a comment

Optional function call results are also correct. Thank you again!

@chicoxyzzy chicoxyzzy merged commit 6dc6b45 into kangax:gh-pages Feb 17, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.