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

feat(eslint-plugin): [prefer-optional-chain] support logical with empty object #4430

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a5d7ec7
feat(eslint-plugin): issue 4395
omril1 Jan 10, 2022
a1dcc4d
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 10, 2022
a5823d3
feat(eslint-plugin): issue 4395
omril1 Jan 11, 2022
65392a8
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 11, 2022
c788fcb
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 13, 2022
bd59eb5
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 15, 2022
6002a5c
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 19, 2022
aac5eb1
fix(eslint-plugin): cr comment
omril1 Jan 19, 2022
155e936
fix(eslint-plugin): more UT and handle ternary
omril1 Jan 24, 2022
13f6e95
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 24, 2022
7f907c5
fix(eslint-plugin): remove comment
omril1 Jan 24, 2022
d9c8120
fix(eslint-plugin): prefer optional chaining over `?? {}).`
omril1 Jan 26, 2022
4065bfe
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Jan 31, 2022
3249b85
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 2, 2022
914d2f9
Legit nitpick
omril1 Feb 9, 2022
6836a74
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 9, 2022
d1650b0
fix(eslint-plugin): prefer optional chaining
omril1 Feb 10, 2022
f355e92
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 12, 2022
75884fb
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 12, 2022
aa33afb
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 13, 2022
3dc29b3
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Feb 17, 2022
2e06f7a
Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining
omril1 Mar 18, 2022
44236bb
feat(eslint-plugin): cr util.getOperatorPrecedence
omril1 Mar 18, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-optional-chain.md
Expand Up @@ -20,6 +20,10 @@ function myFunc(foo: T | null) {
function myFunc(foo: T | null) {
return foo && foo.a && foo.a.b && foo.a.b.c;
}
// or
function myFunc(foo: T | null) {
return (((foo || {}).a || {}).b || {}).c;
}

function myFunc(foo: T | null) {
return foo?.['a']?.b?.c;
Expand Down Expand Up @@ -55,6 +59,9 @@ foo && foo.a && foo.a.b && foo.a.b.c;
foo && foo['a'] && foo['a'].b && foo['a'].b.c;
foo && foo.a && foo.a.b && foo.a.b.method && foo.a.b.method();

(((foo || {}).a || {}).b {}).c;
(((foo || {})['a'] || {}).b {}).c;

// this rule also supports converting chained strict nullish checks:
foo &&
foo.a != null &&
Expand Down
42 changes: 42 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
Expand Up @@ -48,6 +48,48 @@ export default util.createRule({
create(context) {
const sourceCode = context.getSourceCode();
return {
'LogicalExpression[operator=||]'(node: TSESTree.LogicalExpression): void {
Copy link
Member

Choose a reason for hiding this comment

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

The feature does not handle function calls in this way as (foo || {}).bar() can cause a runtime error.

Hmm, interesting point. My hunch would be that we probably want to handle function calls for cases like this:

(someContainer || {}).hasOwnProperty("someKey")

...but I wouldn't consider it a blocker / must-have for this PR. Up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg Care to re-review this? I don't think I'm going to add object built-ins in the PR

Copy link
Member

Choose a reason for hiding this comment

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

You got it, no problem!

const leftNode = node.left;
const rightNode = node.right;
const parentNode = node.parent;
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
const isRightNodeAnEmptyObjectLiteral =
rightNode.type === AST_NODE_TYPES.ObjectExpression &&
rightNode.properties.length === 0;
if (
!isRightNodeAnEmptyObjectLiteral ||
!parentNode ||
parentNode.type !== AST_NODE_TYPES.MemberExpression ||
parentNode.optional
) {
return;
}
context.report({
node: parentNode,
messageId: 'optionalChainSuggest',
suggest: [
{
messageId: 'optionalChainSuggest',
fix: (fixer): TSESLint.RuleFix => {
const leftNodeText = context.getSourceCode().getText(leftNode);
omril1 marked this conversation as resolved.
Show resolved Hide resolved
const maybeWrappedLeftNode =
leftNode.type === AST_NODE_TYPES.LogicalExpression
? `(${leftNodeText})`
: leftNodeText;
const propertyToBeOptionalText = context
.getSourceCode()
omril1 marked this conversation as resolved.
Show resolved Hide resolved
.getText(parentNode.property);
const maybeWrappedProperty = parentNode.computed
? `[${propertyToBeOptionalText}]`
: propertyToBeOptionalText;
return fixer.replaceTextRange(
parentNode.range,
`${maybeWrappedLeftNode}?.${maybeWrappedProperty}`,
);
},
},
],
});
},
[[
'LogicalExpression[operator="&&"] > Identifier',
'LogicalExpression[operator="&&"] > MemberExpression',
Expand Down
94 changes: 94 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts
Expand Up @@ -146,6 +146,11 @@ const baseCases = [

ruleTester.run('prefer-optional-chain', rule, {
valid: [
'foo || {};',
'foo || ({} as any);',
'(foo || {})?.bar;', // This might seem stupid, but I'm not sure if we want to show suggestion for it
omril1 marked this conversation as resolved.
Show resolved Hide resolved
'(foo || { bar: 1 }).bar;',
'(undefined && (foo || {})).bar;',
'foo && bar;',
'foo && foo;',
'foo || bar;',
Expand Down Expand Up @@ -501,5 +506,94 @@ foo?.bar(/* comment */a,
},
},
},
{
omril1 marked this conversation as resolved.
Show resolved Hide resolved
code: 'const foo = (bar || {}).baz;',
errors: [
{
messageId: 'optionalChainSuggest',
column: 13,
endColumn: 28,
suggestions: [
{
messageId: 'optionalChainSuggest',
output: 'const foo = bar?.baz;',
},
],
},
],
},
{
code: '(foo.bar || {})[baz];',
errors: [
{
messageId: 'optionalChainSuggest',
column: 1,
endColumn: 21,
suggestions: [
{
messageId: 'optionalChainSuggest',
output: 'foo.bar?.[baz];',
},
],
},
],
},
{
// Currently it does not suggest a fix for nested optional with empty object
// It shows 2 suggestions, one for the outer object and one for the inner object
omril1 marked this conversation as resolved.
Show resolved Hide resolved
code: '((foo1 || {}).foo2 || {}).foo3;',
errors: [
{
messageId: 'optionalChainSuggest',
column: 1,
endColumn: 31,
suggestions: [
{
messageId: 'optionalChainSuggest',
output: '(foo1 || {}).foo2?.foo3;',
},
],
},
{
messageId: 'optionalChainSuggest',
column: 2,
endColumn: 19,
suggestions: [
{
messageId: 'optionalChainSuggest',
output: '(foo1?.foo2 || {}).foo3;',
},
],
},
],
},
{
code: '(foo || undefined || {}).bar;',
errors: [
{
messageId: 'optionalChainSuggest',
suggestions: [
{
messageId: 'optionalChainSuggest',
output: '(foo || undefined)?.bar;',
},
],
},
],
},
{
code: noFormat`(undefined && foo || {}).bar;`,
errors: [
{
messageId: 'optionalChainSuggest',
suggestions: [
{
messageId: 'optionalChainSuggest',
output: '(undefined && foo)?.bar;',
},
],
},
],
},
],
});