search

tailwindlabs / tailwindui-issues

A place to report bugs discovered in Tailwind UI.
236 stars 4 forks source link

Typescript const vs let #1569

Closed garyhuntddn closed 6 months ago

garyhuntddn commented 6 months ago

Lots of the catalyst components - e.g. table.tsx

Describe the bug It's considered normal practice to use const instead of let if a variable is not going to be changed. Eslint has picked up a number of places in the catalyst components where this isn't the case.

To Reproduce Steps to reproduce the behavior:

  1. Ensure that eslint is installed (normal for a nextjs app)
  2. Add { ... "rules": { .... "prefer-const": "error" } } to .eslintrc.json
  3. Run eslint ( e.g. npm run lint for a normal nextjs app)

Expected behavior That variables defined as let are changed after they are initially declared, and if they aren't then they should be declared const.

Additional context Just trying to suggest some improvements to the code as it's going to be code that we include in our own projects

RobinMalfait commented 6 months ago

Hey!

Totally get what you are saying, we typically have the convention to only use const in top-level module scope and everything else is let.

This is one is leaning to one of those subjective matters where some people prefer const some let, some people prefer semicolons some don't, some prefer tabs some spaces, some prefer named exports some default exports and so on.

We basically picked a convention that is consistent throughout the codebase. The good part is that once you copy the components in your project you can choose the conventions you want.

I think we will keep it like this for now, but if more people want this change then we can definitely revisit this.

Hope this helps!