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

Don't lose optional chaining with objectAt in autofix for no-array-prototype-extensions rule #1687

Conversation

52052100
Copy link
Contributor

Summary

Add optional chaining support to objectAt auto-fixer.

Detail

Right now the objectAt auto-fixer in no-array-prototype-extensions will remove the optional chaining operator(?.). So this PR will use callExpressionNode.callee.optional to check the optional chaining use case and keep the optional chaining operator.

For example, the following template
const response = this.demoResponses?.objectAt(questionIndex);

Before:
const response = this.demoResponses[questionIndex];

After:
const response = this.demoResponses?.[questionIndex];

Testing Done

Add two new test cases to check the optional chaining use case.
All test cases passed.

@bmish bmish added the Bug label Nov 30, 2022
@bmish bmish changed the title Layang/no array prototype extensions/autofix object at Don't lose optional chaining with objectAt in autofix for no-array-prototype-extensions rule Nov 30, 2022
@bmish
Copy link
Member

bmish commented Nov 30, 2022

Do you think this is an issue with any of the other functions in the autofixer?

Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
@52052100
Copy link
Contributor Author

Do you think this is an issue with any of the other functions in the autofixer?

yeah, had a discussion with @tgvrssanthosh , this issue might be not specific to objectAt but across all of them. Need to do some tests. We can have more general support for optional chaining use cases.

@bmish
Copy link
Member

bmish commented Nov 30, 2022

Feel free to use fix the issues with the other functions in separate PRs or in the same PR, up to your preference. For the next PR you could try to address multiple functions if you want. Looks like this PR is almost finished except you need to fix lint (and I recommend enabling trim trailing whitespace in your editor).

@52052100
Copy link
Contributor Author

Feel free to use fix the issues with the other functions in separate PRs or in the same PR, up to your preference. For the next PR you could try to address multiple functions if you want. Looks like this PR is almost finished except you need to fix lint (and I recommend enabling trim trailing whitespace in your editor).

Sure, no problem! I will try to fix other functions in separate PRs to make sure don't break the function. Thanks for the suggestion! Just updated the code format and push.

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@bmish bmish merged commit 7493c80 into ember-cli:master Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants