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

strange fix for jsx-no-leaked-render #3297

Closed
lukesmurray opened this issue May 26, 2022 · 9 comments · Fixed by #3299
Closed

strange fix for jsx-no-leaked-render #3297

lukesmurray opened this issue May 26, 2022 · 9 comments · Fixed by #3299

Comments

@lukesmurray
Copy link

Input File

import { css } from "twin.macro";

export interface sortIndicatorProps {
  direction?: "up" | "down";
  removeFromLayout?: boolean;
  className?: string;
}

export const sortIndicatorMarginLeft = "-1em";

export function SortIndicator({
  direction,
  removeFromLayout = true,
  className,
}: sortIndicatorProps) {
  return (
    <span
      className={className}
      css={[
        css`
          font-size: 0.8em;
          &:first-of-type {
            padding-right: 0.5ch;
          }
        `,
        removeFromLayout &&
          css`
            position: absolute;
            margin-left: ${sortIndicatorMarginLeft};
          `,
      ]}>
      {direction ? (direction === "down" ? "▼" : "▲") : ""}
    </span>
  );
}

output file

import { css } from "twin.macro";

export interface sortIndicatorProps {
  direction?: "up" | "down";
  removeFromLayout?: boolean;
  className?: string;
}

export const sortIndicatorMarginLeft = "-1em";

export function SortIndicator({
  direction,
  removeFromLayout = true,
  className,
}: sortIndicatorProps) {
  return (
    <span
      className={className}
      css={[
        css`
          font-size: 0.8em;
          &:first-of-type {
            padding-right: 0.5ch;
          }
        `,
        removeFromLayout &&
          css`
            position: absolute;
            margin-left: ${sortIndicatorMarginLeft};
          `,
      ]}>
      {/* lol what is this ↓↓↓↓↓ */}
      {!!!!!!!!!!!!!!!!!!!!direction && direction === "down" && "▼"}
    </span>
  );
}

eslint rule setup

    "react/jsx-no-leaked-render": [
      // enforce that values are coerced to booleans in jsx conditionals
      "error",
      {
        validStrategies: ["coerce"], // coerce to boolean e.g. `<div>{count && title}</div>` becomes `<div>{!!count && title}</div>`
      },
    ],
@lukesmurray
Copy link
Author

Every time I run eslint --fix the exclamation marks get longer 😂

      {!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!direction &&
        direction === "down" &&
        "▼"}

@lukesmurray
Copy link
Author

lukesmurray commented May 26, 2022

In general fix probably shouldn't change behavior. If you have validStrategies: ["coerce"] the autofix removes the end of the ternary.

For example I have a component that displays Loading {message} or Loading based on a condition. The autofix removes the branch that displays Loading

input

  containerName.length > 0
    ? `Loading ${containerName.toLocaleLowerCase()}`
    : "Loading"

output

  !!containerName.length > 0 &&
  `Loading ${containerName.toLocaleLowerCase()}`

This is a small example. It will also remove entire component trees.

@lukesmurray
Copy link
Author

The exclamation marks show up on strings and nested values.

                            !!!!!!!!!!!!!!!!!!column.isSorted && column.isSortedDesc && "down"

@ljharb
Copy link
Member

ljharb commented May 26, 2022

lol this is clearly a bug. cc @Belco90

@lukesmurray
Copy link
Author

let me know if you have trouble reproducing. to be clear there are two bugs here. the unnecessary exclamation marks and the deletion of branches from ternary expressions

@Belco90
Copy link
Contributor

Belco90 commented May 26, 2022

the unnecessary exclamation marks and the deletion of branches from ternary expressions

No idea what's causing the former! But the latter it's easy to fix. I'm on it.

@ljharb
Copy link
Member

ljharb commented May 26, 2022

@Belco90 thanks! i'd love to publish a patch release soon, and i'd love to include fixes for the latest no-leaked-render issues :-)

@Belco90
Copy link
Contributor

Belco90 commented May 26, 2022

@ljharb I got the two issues reported here fixed + #3292 partially fixed. Still figuring out how to prevent reporting when TS types are involved.

@Belco90
Copy link
Contributor

Belco90 commented May 26, 2022

Sorry, I forgot to mention: I can create a PR with those fixes while I figure out the last one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants