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

Not working properly with class-variance-authority (CVA) #36

Closed
5 of 9 tasks
FadyAmir223 opened this issue May 23, 2024 · 5 comments
Closed
5 of 9 tasks

Not working properly with class-variance-authority (CVA) #36

FadyAmir223 opened this issue May 23, 2024 · 5 comments
Labels
bug Something isn't working confirmed

Comments

@FadyAmir223
Copy link

FadyAmir223 commented May 23, 2024

Type

  • Incorrect Class Name Extraction
  • Incorrect JavaScript Obfuscation
  • Incorrect HTML Obfuscation
  • Incorrect CSS Obfuscation
  • Others

Checklist

  1. Updated the package to the latest version
  2. Read all documentation
  3. No related issue
  4. Meaningful issue title

Environment

  • Package Version: v.2.2.15
  • Node Version: v.18.18.1

Describe the bug
shadcn-ui uses CVA which the package doesn't recognize

const labelVariants = cva(
  'text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70',
)

the problem lies in the assumption that classes will be inlined after "className"

if (t.isIdentifier(path.node.key) && path.node.key.name === "className")

but in the bundle it puts the the string in a function then call it below

      const l = (0, i.j)(
        'text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70',
      )
      let d = a.forwardRef((e, t) => {
        const { className: r, ...i } = e
        return (0, n.jsx)(s.f, { ref: t, className: (0, o.cn)(l(), r), ...i })
      })

more complex example

const c = (0, n.j)('inline-flex items-center justify-center', {
  variants: {
    variant: {
      default: 'bg-primary text-primary-foreground',
      destructive: 'bg-destructive text-destructive-foreground',
    },
    size: {
      default: 'h-9 px-4 py-2',
    },
  },
  defaultVariants: { variant: 'default', size: 'default' },
})
const l = i.forwardRef(
  ({ className: e, variant: t, size: r, asChild: n = !1, ...i }, l) => {
    const d = n ? a.g7 : 'button'
    return s.jsx(d, {
      className: (0, o.cn)(c({ variant: t, size: r, className: e })),
      ref: l,
      ...i,
    })
  },
)

it can be avoided by turning off removeOriginalCss option but I suggested the simplify variation like medium to reduce the size of css file, doing that defeats the purpose of the package

a suggested solution is to compare the conversion.json keys with the string boundary after ensuring that text inside is tailwind classes, but I'm sure you can come up with a better idea :)

Config

module.exports = {
  enable: true,
  mode: 'simplify',
  refreshClassConversionJson: process.env.NODE_ENV !== 'production',
  removeOriginalCss: true,
  blackListedFolderPaths: [
    './.next/cache',
    /\.next\/server\/pages\/api/,
    /_document..*js/,
    /_app-.*/,
    /__.*/,
  ],
}
@FadyAmir223 FadyAmir223 added the bug Something isn't working label May 23, 2024
@soranoo
Copy link
Owner

soranoo commented May 23, 2024

Sorry, I don't think it is possible to fix since there is no identifiable pattern. To prevent breaking the JavaScript accidentally, a strict className searching strategy has been implemented inside the non-AST based obfuscation.

Solutions

  1. Set enableJsAst to true inside the config.
    The className tracing strategy will be applied by enabling this option. This strategy enables the obfuscator to find out all className related variables.

For Future Reference

The line you mentioned (the following) is inside the js-ast.ts and is not relevant to the config you provided.

if (t.isIdentifier(path.node.key) && path.node.key.name === "className")

The functions inside js-ast.ts won't be activated when enableJsAst=false(default config)

@soranoo soranoo pinned this issue May 23, 2024
@soranoo soranoo closed this as completed May 24, 2024
@FadyAmir223
Copy link
Author

FadyAmir223 commented May 24, 2024

If there is no pattern we can create it, we may require developers to add marker like "next-css-obfuscator" in strings used by CVA then remove it in the bundle and obfuscate

@soranoo
Copy link
Owner

soranoo commented May 24, 2024

Then removeMarkersAfterObfuscated can be your friend~

@FadyAmir223
Copy link
Author

I tried to apply partial obfuscation to the whole project, but for some reason cn() (tailwindMerge) didn't work as expected (e.g. rounded-full didn't overwrite rounded-md) so I think the solution is to enable a marker alongside the full obfuscation to cover the blind spots like CVA

@soranoo
Copy link
Owner

soranoo commented May 25, 2024

You can achieve this using the following config,

module.exports = {
enableObfuscateMarkerClasses = true,
obfuscateMarkerClasses = [".jsx", "{your_custom_key}"]
// rest of your config...
}

The "class" .jsx(js pattern) is the default marker for searching class names in non-AST based full obfuscation and it won't be removed after obfuscation is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working confirmed
Projects
None yet
Development

No branches or pull requests

2 participants