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 clash when using no-useless-undefined, no-null, TypeScript and React.createContext #1684

Closed
kachkaev opened this issue Jan 9, 2022 · 3 comments · Fixed by #1688
Closed
Labels

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Jan 9, 2022

I added plugin:unicorn/recommended to a few side projects and my code got noticeably more coherent. Great plugin folks!

I have noticed one interesting clash between linting rules and wonder if it can be addressed by the plugin. Here’s my original TypeScript code (before Unicorn):

import * as React from "react";

interface MyContextValue {
  x: number;
  y: string;
};

export const MyContext = React.createContext<MyContextValue | null>(null);

export const useMyContext = (): MyContextValue => {
  const result = React.useContext(MyContext);
  if (result === null) {
    throw new Error("No MyContext value available");
  }
  return result;
}

Once I added recommended Unicorn rules, I got saw near error near export const MyContext): Use `undefined` instead of `null` (unicorn/no-null). Me: OK,

 import * as React from "react";

 interface MyContextValue {
   x: number;
   y: string;
 };

-export const MyContext = React.createContext<MyContextValue | null>(null);
+export const MyContext = React.createContext<MyContextValue | undefined>(undefined);

 export const useMyContext = (): MyContextValue => {
   const result = React.useContext(MyContext);
-  if (result === null) {
+  if (result === undefined) {
     throw new Error("No MyContext value available");
   }
   return result;
 }

Unicron: Do not use useless `undefined` (unicorn/no-useless-undefined). Me: OK,

-export const MyContext = React.createContext<MyContextValue | undefined>(undefined);
+export const MyContext = React.createContext<MyContextValue | undefined>();

TypeScript: Expected 1 arguments, but got 0.. Me: React typings must be incomplete and I should submit a PR that allows omitting undefined. React typings:

    function createContext<T>(
        // If you thought this should be optional, see
        // https://github.com/DefinitelyTyped/DefinitelyTyped/pull/24509#issuecomment-382213106
        defaultValue: T,
    ): Context<T>;

Link: DefinitelyTyped/DefinitelyTyped#24509 (comment). Me: Hmm, this looks like a dead end 😅

Assuming that we can’t easily update React typings, looks like we should either:

  • allow null in createContext / React.createContext
    or
  • stop marking undefined as useless if it’s an argument of createContext / React.createContext.

In the meantime, my workaround is:

+ // eslint-disable-next-line unicorn/no-useless-undefined
 const MyContext = React.createContext<MyContextValue | undefined>(undefined);

or:

 // .eslintrc.cjs
 module.exports = {
   extends: [
     "...",
     "plugin:unicorn/recommended",
   ],
+  rules: {
+    "unicorn/no-useless-undefined": "off", // https://github.com/sindresorhus/eslint-plugin-unicorn/issues/1684
+  },
 };

For the record, I am using eslint-plugin-unicorn@40.0.0 and @types/react@17.0.38. WDYT folks?

@kachkaev kachkaev added the bug label Jan 9, 2022
@fisker
Copy link
Collaborator

fisker commented Jan 10, 2022

I think

stop marking undefined as useless if it’s an argument of createContext / React.createContext.

Make sense.

@fisker
Copy link
Collaborator

fisker commented Jan 10, 2022

In case you missed, we also have checkArguments option in no-useless-undefined rule, https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-useless-undefined.md#checkarguments

@kachkaev
Copy link
Contributor Author

👋 Thanks for sharing your thoughts @fisker! See #1688 👀

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 a pull request may close this issue.

2 participants