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

docs: add ESLint configuration section to installation docs #449

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

ollema
Copy link
Contributor

@ollema ollema commented Nov 15, 2023

with this commit, a .eslintrc file should be added to the .../components/ui folder when initialising a new project with npx shadcn-svelte@latest init.

I write should because I do not know how to test this 😅 so I have coded it "blind"

I added this because it's a common problem that people run into, this solution works without modifying existing project wide .eslintrc.cjs files.

a downside is that this is added without checking if the project uses eslint to begin with. a check for that could be added I guess!

feel free to close if you dislike this idea - no problem

Copy link

changeset-bot bot commented Nov 15, 2023

⚠️ No Changeset found

Latest commit: f0c70a7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Nov 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shadcn-svelte ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2023 0:15am

@huntabyte
Copy link
Owner

Hey @ollema, I thought about this recently but ultimately decided against it, because as you mentioned, we have to make a lot of assumptions that are outside the scope of this project. We don't want to be in the business of modifying/setting ESLint configs on the project's behalf.

One thing we could do instead is add a section to the installation page of the docs, which would include something like 'Recommended ESLint Config' and include those pieces and why they are recommended!

Will also loop in @AdrianGonz97 for his thoughts on this as well!

@huntabyte
Copy link
Owner

Hey @ollema, are you wanting to add this to the docs or should we close this issue for now?

@ollema
Copy link
Contributor Author

ollema commented Nov 24, 2023

Hey @ollema, are you wanting to add this to the docs or should we close this issue for now?

sure, I can do that. I missed that suggestion in your original message

@niemyjski
Copy link

We shouldn't be adding any eslint overrides to the components folder. We should fix any default rule errors in this code base.

@huntabyte
Copy link
Owner

huntabyte commented Nov 29, 2023

It's not an issue with this code base, it's an issue with the default eslint rules, which tbh will be resolved in Svelte 5 but in the meantime requires you to ignore the $$Events & $$Props if unused. Otherwise you looks intellisense/type safety for the HTMLAttributes props on certain elements that don't have other props.

Here's an eslint config you can use to ignore these in your codebase, but we're not going to be modifying/messing with anyone's eslint config.

ESLint Config

@ollema ollema force-pushed the add-eslintrc-with-init-cmd branch from d1610e2 to f0c70a7 Compare December 9, 2023 00:11
@ollema
Copy link
Contributor Author

ollema commented Dec 9, 2023

ready for review @huntabyte

@ollema ollema changed the title feat: add .eslintrc file with init cli command docs: add ESLint configuration section to installation docs Dec 9, 2023
Copy link
Owner

@huntabyte huntabyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks man! Love the multiple options!

@huntabyte huntabyte merged commit 7b191ea into huntabyte:main Dec 11, 2023
3 checks passed
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

Successfully merging this pull request may close these issues.

3 participants