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

Linter errors when using TreeViewApi #52

Open
fernandocanizo opened this issue May 9, 2024 · 6 comments
Open

Linter errors when using TreeViewApi #52

fernandocanizo opened this issue May 9, 2024 · 6 comments

Comments

@fernandocanizo
Copy link

fernandocanizo commented May 9, 2024

Hello, I'm trying to use TreeViewApi and Eslint complains about several things:

  13:3  error  'useMemo' is defined but never used         @typescript-eslint/no-unused-vars
  183:6  error  'className' is missing in props validation  react/prop-types
  225:5  error  'ref' is defined but never used             @typescript-eslint/no-unused-vars
  293:7  error  'className' is missing in props validation  react/prop-types
  294:7  error  'handleSelect' is defined but never used    @typescript-eslint/no-unused-vars
  338:6  error  'className' is defined but never used       @typescript-eslint/no-unused-vars
  338:6  error  'className' is missing in props validation  react/prop-types

I cloned the repo and opened the file and I get a different set of errors and warnings. I wonder what's the difference and which set of rules provides a better DX and reassurance against possible errors.

@gaurangrshah
Copy link
Collaborator

Hey @fernandocanizo thanks for bringing this to our attn.

Can you possibly provide a minimal reproduction? Codesandbox link or otherise would be great.

But at first glance...
It seems like the defined but never used warnings are valid (so useMemo, and the ref are both value errors)

  • we can address that internally.

Same goes for the handleSelect fn (but we should check if there was a reason why we wanted to allow this in the api).

Which brings us to the classNames, where you're getting a missing in props validation issue that somehow seems to me like it might be unique to your project's eslint config.

But I did see similar issues with a few workarounds on a few different repos:

If you're using vscode, my best is likely this suggestion here: shadcn-ui/ui#120 (comment)

Will be glad to dig in further when you are able to provide a reproduction. Thank you.

@fernandocanizo
Copy link
Author

Hi @gaurangrshah, thanks for the follow up and the links. I also came across some of those when I started looking into it, as I found, after publishing this issue, that the problem appears also in some base components from Shadcn/ui. So after reading all I could find, now I understand the problem is related to react/prop-types and is pervasive through all the chain of dependencies up to jsx-eslint/eslint-plugin-react.

The problem is that most probably anyone doing modern development these days would probably have an Eslint configured with plugin react/recommended which brings in react/prop-types, which produces this error. I'm developing a Remix project, and the scaffolder brings in the plugin by default (just check pnpx create-remix).

To me the best argument to disable the rule, which I disliked at first, is that react/prop-types doesn't add anything on a TS project.

Regarding the issue surfacing in shadcn-extension, there are two possibilities:

  1. either you clearly document this is not a shadcn-extension problem and recommend to ignore the linter rule and provide the config changes to be made.
  2. you start using the Eslint plugin too and adding the missing className and others that may appear as intersection types to the definitions:
// ...
& { className?: string }

I did a quick attempt at making it reproducible: I wanted to use the relevant part of Remix default Eslint configuration in shadcn-extension repository, as that would be much simpler than creating a new project and adding TreeView, but couldn't reproduce it. So it may require more investment from my part.

Regarding the unused values, some appear (useMemo, ref) with current (f5bebda) shadcn-extension rules, however other unused, like handleSelect, only appear on my project. Again, I will need to go deeper in order to provide a way to reproduce.

@gaurangrshah
Copy link
Collaborator

Hey @fernandocanizo thank you so much for doing the research and reporting back. I did not originally realize that this was related to a Remix project. We definitely want to be able to support all of the frameworks that ShadCn itself actually supports.

After going over your findings I whole heartedly agree with you. Personally I am a big fan of eslint and look forward to adding it to this project.

We are actually in the middle of a refactor to a monorepo setup with some new additions. So we will be taking the time to address some these issues as well as we work towards a v1 release. We'll be making some announcements shortly in regards to these new updates

I'd like to keep our channel of communication open if that interests you we just opened our discord up to the public and would like to extend the invitation to you as well. https://discord.gg/Fq6n4dYU

Thank you again for your due diligence and taking out the time to repot back.

@fernandocanizo
Copy link
Author

Thanks for the invitation, coming there as soon as I hit the button to send this comment :)

Just to clarify: it's a Remix project, but the issue is not Remix related specifically. It's just that Remix' scaffolder brings in an Eslint configuration with "plugin:react/recommended" which in turn brings the react/prop-types plugin.

@alamenai
Copy link

alamenai commented Jun 8, 2024

Hi @gaurangrshah , could we add a bot for closing issue that are stale or they don't have activity for long time?

@gaurangrshah
Copy link
Collaborator

Hi @gaurangrshah , could we add a bot for closing issue that are stale or they don't have activity for long time?

That makes sense, I've seen some used before, but never used one myself.

If you know of any specific ones that are good can u drop me some links to research in discord? If not I'll do some digging early next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants