francoismassart / eslint-plugin-tailwindcss

ESLint plugin for Tailwind CSS usage
https://www.npmjs.com/package/eslint-plugin-tailwindcss
MIT License
1.38k stars 65 forks source link

[BUG] Classes not detected inside objects with computed property (using `clsx` callee) #331

Open redocean10 opened 3 months ago

redocean10 commented 3 months ago

Describe the bug The plugin won't identify classes inside objects that have a computed property (object[property] or object.property), so doesn't check for any rules (classnames-order in this case).

To Reproduce

// .eslintrc.cjs
module.exports = {
  extends: [
    "plugin:tailwindcss/recommended",
  ],
  settings: {
    tailwindcss: {
      callees: ["clsx"],
    }
  },
  rules: {
    "tailwindcss/no-custom-classname": "off",
     },
  }
// Alert.jsx
import clsx from 'clsx'

let type = 'error'
const alertClass = clsx(
  {
    success: 'border-green-400 bg-green-100 text-green-700',
    warning: 'text-yellow-700 border-yellow-400 bg-yellow-100',
    error: 'bg-red-100 border-red-400 text-red-700',
  }[type],
)

Expected behavior The plugin should detect the classes and warn of the invalid order.

Screenshots With the computed property (no linting): image

Without property (linting works fine): image

Environment (please complete the following information):

Current fix/ workaround:

I managed to fix this issue for the classnames-order rule, by updating the callExpressionVisitor function inside classnames-order.js:

const callExpressionVisitor = function (node) {
  const calleeStr = astUtil.calleeToString(node.callee);
  if (callees.findIndex((name) => calleeStr === name) === -1) {
    return;
  }

  node.arguments.forEach((arg) => {
    if (arg.type === 'MemberExpression' && arg.object.type === 'ObjectExpression') {
      arg.object.properties.forEach((prop) => {
        if (prop.value.type === 'Literal') {
          sortNodeArgumentValue(node, prop.value);
        }
      });
    } else {
      sortNodeArgumentValue(node, arg);
    }
  });
};
kachkaev commented 3 months ago

Hmm not sure if creating an object and then picking a single element from it is an efficient way of achieving what you are after. If there is ever a fix, it will encourage odd-looking clsx calls which may look confusing for collaborators.

Either way, you can do this in the meantime:

const alertClassName = clsx(
  "", // ← common class names go here
  type === "success" && "border-green-400 bg-green-100 text-green-700",
  type === "warning" && "text-yellow-700 border-yellow-400 bg-yellow-100",
  type === "error" && "bg-red-100 border-red-400 text-red-700"
);

Or, even better, you can leverage cva:

import { cva } from "class-variance-authority";

const alertVariants = cva(
  "", // ← common class names go here
  {
    variants: {
      type: {
        success: "border-green-400 bg-green-100 text-green-700",
        warning: "text-yellow-700 border-yellow-400 bg-yellow-100",
        error: "bg-red-100 border-red-400 text-red-700",
      },
    },
  }
);

const alertClassName = alertVariants({ type });

Both alternatives are covered by this plugin.

redocean10 commented 3 months ago

Thanks for your suggestions. It seemed like a simple way to handle variants to me, but I agree it doesn't look right. I'm using a cleaner approach now that works with the plugin, so maybe this bug was my error.