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

Rule proposal: prefer-optional-chaining #429

Open
jorgegonzalez opened this issue Nov 5, 2019 · 20 comments
Open

Rule proposal: prefer-optional-chaining #429

jorgegonzalez opened this issue Nov 5, 2019 · 20 comments

Comments

@jorgegonzalez
Copy link

jorgegonzalez commented Nov 5, 2019

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

// first and second may be null or undefined:
let nestedProp = obj.first && obj.first.second;

// The line above should be transformed to this:
let nestedProp = obj.first?.second;
@yakov116
Copy link
Contributor

yakov116 commented Nov 5, 2019

@jorgegonzalez Experimental. Expect behavior to change in the future.
maybe in a year or 2

@jorgegonzalez
Copy link
Author

It's in Stage 3 and already landed in Babel and TypeScript 🤷‍♂

@fisker fisker reopened this Nov 5, 2019
@fisker
Copy link
Collaborator

fisker commented Nov 5, 2019

why close? I think its great, and already using

@sindresorhus
Copy link
Owner

Big 👍, but we have to wait for ESLint to add support for it in their parser first.

@jorgegonzalez
Copy link
Author

@fisker
Copy link
Collaborator

fisker commented Dec 6, 2019

Optional chaining is now stage 4!

@teppeis

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@teppeis

This comment has been minimized.

@horacio-penya
Copy link

@EvgenyOrekhov
Copy link
Contributor

Support for optional chaining was added in ESLint 7.5.0.

Also, @typescript-eslint/prefer-optional-chain can be used in JavaScript projects. @typescript-eslint/prefer-optional-chain is not autofixable though.

Interesting fact: quite a few of TypeScript rules can be used in JavaScript projects, see https://github.com/EvgenyOrekhov/eslint-config-hardcore#hardcorets-for-js.

@sindresorhus sindresorhus changed the title Rule Proposal: prefer-optional-chaining Rule proposal: prefer-optional-chaining Sep 25, 2020
@fregante
Copy link
Collaborator

fregante commented Oct 1, 2020

Why not close this since @typescript-eslint/prefer-optional-chain exists?

@EvgenyOrekhov
Copy link
Contributor

@fregante

  1. @typescript-eslint rule doesn't provide an autofix.
  2. @typescript-eslint is usually used for linting TypeScript, whereas eslint-plugin-unicorn targets mostly JavaScript users.

As a side note, I recently discovered this project: https://github.com/coderaiser/putout. It has an ESLint plugin, and it provides dozens of autofixes (some of them are quite advanced), including plugin-apply-optional-chaining.

@fregante
Copy link
Collaborator

fregante commented Oct 1, 2020

  • @typescript-eslint rule doesn't provide an autofix.

If safe, it could be added there, why write a whole rule from scratch?

@typescript-eslint is usually used for linting TypeScript

But as you mentioned it could be used on JS files as well.


Edit: I tried putout on Refined GitHub and mostly it just broke code and dropped whole lines of very clear code without any reason. I wouldn't trust it. Astounding 90% unwanted/breaking change rate.

@sindresorhus
Copy link
Owner

Seems like ESLint will add this rule at some point, so there's no point in adding it here. eslint/eslint#13430

@bradzacher
Copy link

There is a reason that the @typescript-eslint rule no longer has an autofix.
It is not possible to have a rule that autofixes to optional chaining safely[1].

You can only detect these cases with a type-aware lint rule.

For example:

function foo(arg) {
  return arg && arg.foo;
}

Should this be converted into an optional chain? Seems like it... until you add more information about the expected types:

function foo(arg: false | { foo: string }) {
  return arg && arg.foo;
}

foo(false);
foo({ foo: 'a' });

Now it is no longer correct to make this an optional chain.
In a pure JS world - this is technically a "safe" change (nothing will throw at runtime), but it changes the return value of the function from string | false to string | undefined.
In a TS codebase - this change would also break the build, because .foo does not exist on the type false.

Is this code a weird pattern? For sure.
Is it valid code that 100% exists in the real world? Definitely.

There are numerous other examples that you could dig out of the issues filed on https://github.com/typescript-eslint/typescript-eslint.


[1] To clarify for people - an ESLint autofixer must be safe - meaning it mustn't break the build or change runtime semantics of code.

This is why the @typescript-eslint rule uses a suggestion fixer.
Unlike autofixers, suggestion fixers can produce "broken" code because there's no way to auto-apply suggestion fixes -- they can only be manually applied by an engineer.

The @typescript-eslint rule originally had an autofixer - but many users complained about broken code because of it.

@fregante
Copy link
Collaborator

Yeah in this case the correct auto fix would be arg?.foo ?? false, which defeats the point of using the operator and confuses people about what the solution should be.

@Samuel-Therrien-Beslogic

Copied from #2230
Some more optional chaining not mentioned in OP:

Fail

foo === undefined ? foo : foo.bar // Autofix to `foo?.bar`
foo == undefined ? undefined : foo.bar // Autofix to `foo?.bar`
foo == null ? undefined : foo.bar // Autofix to `foo?.bar`

Pass

foo == null ? foo : bar
foo == undefined ? foo : bar
// Technically these can change the return type from `null` to `undefined`. So I'm not sure if they should pass or fail with suggestion
foo == undefined ? foo : foo.bar // Suggests `foo?.bar`
foo == null ? foo : foo.bar // Suggests `foo?.bar`

Additional Info

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v46.0.1/docs/rules/no-negated-condition.md already takes cares of the != cases so I didn't list them here.

ts(5076) prevents mixing &&/|| with ?? without parenthesis. So some autofix might require adding parenthesis.

@bradzacher
Copy link

https://typescript-eslint.io/rules/prefer-optional-chain/

Note that since this issue in v6 of our plugin we did a massive refactoring of the rule. It now uses type information because users found that there were simply too many false positives.

By using type information it means the rule can also offer safe autofixes.

My 2c: this issue should be closed because there's no way that this plugin can provide a syntax-only rule that offers reports without false positives or an autofixer that won't change runtime behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants