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

fix(typescript): fix eslint_d not looking for .eslintrc file #558

Closed
wants to merge 2 commits into from

Conversation

Dartv
Copy link

@Dartv Dartv commented Sep 8, 2023

📑 Description

Adds support for .eslintrc config file. Otherwise eslint_d is not initialized for projects that use the file.

ℹ Additional Information

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Review Checklist

Does this PR follow the [Contribution Guidelines](development guidelines)? Following is a partial checklist:

Proper conventional commit scoping:

  • If you are adding a new plugin, the scope would be the name of the category it is being added into. ex. feat(utility): added noice.nvim plugin

  • If you are modifying a pre-existing plugin or pack, the scope would be the name of the plugin folder. ex. fix(noice-nvim): fix LSP handler error

  • Pull request title has the appropriate conventional commit type and scope where the scope is the name of the pre-existing directory in the project as described above

  • README is properly formatted and uses fenced in links with <url> unless they are inside a [title](url)

  • Proper usage of opts table rather than setting things up with the config function.

@Uzaaft
Copy link
Member

Uzaaft commented Sep 8, 2023

Based upon: https://eslint.org/docs/latest/use/configure/configuration-files, .eslintrc is not a file name it supports.

@Dartv
Copy link
Author

Dartv commented Sep 8, 2023

Interesting, I have a project with .eslintrc file and this PR fixes the issue for me.
Changing the file on a large project with lots of people may raise questions. That's why I'd rather have a fix at a plugin level

@Uzaaft
Copy link
Member

Uzaaft commented Sep 8, 2023

I’d still need a source for the .eslintrc file which States that eslint looks after that file before I merge this

@Uzaaft
Copy link
Member

Uzaaft commented Sep 8, 2023

This pr might fix the issue with eslint not spawning. But does it use the settings from the file?

@Dartv
Copy link
Author

Dartv commented Sep 8, 2023

Yes, it works with .eslintrc config file. I just created a new project with minimal repro.

@Dartv
Copy link
Author

Dartv commented Sep 8, 2023

@Uzaaft
Copy link
Member

Uzaaft commented Sep 8, 2023

Eslint tests use .eslintrc file if you need more proof eslint/eslint@cac45d0/tests/fixtures/config-extends/.eslintrc#L4

You've misunderstood that. Take a look at the package.json file.

@Uzaaft
Copy link
Member

Uzaaft commented Sep 8, 2023

Yes, it works with .eslintrc config file. I just created a new project with minimal repro.

I got that it work. But I'm hesitant to add something that's not mentioned in the official docs

@owittek
Copy link
Member

owittek commented Sep 9, 2023

It's likely that using .eslintrc without a file ending is a legacy thing, pretty sure I've seen it before. I honestly don't see a reason to not merge this as I don't think people just have random files with that name lying in their project folder

@Uzaaft
Copy link
Member

Uzaaft commented Sep 9, 2023

It's likely that using .eslintrc without a file ending is a legacy thing, pretty sure I've seen it before. I honestly don't see a reason to not merge this as I don't think people just have random files with that name lying in their project folder

Even if it's legacy? I would've just left it to the users to add it to their configs instead.

@owittek
Copy link
Member

owittek commented Sep 9, 2023

what's the downside of just adding it? I think the upside is that there won't be any issues/PRs for this exact issue in the future as legacy projects still account for a big percentage of total projects so it's not unlikely to run into this again

@owittek
Copy link
Member

owittek commented Sep 9, 2023

to add onto this I'd personally even be fine with a regex match of that sort: ^\.eslintrc(\.\w+)?$

@taskylizard
Copy link
Contributor

It's likely that using .eslintrc without a file ending is a legacy thing, pretty sure I've seen it before.

Just wanted to chime in and say that it is, and want to stand with merging of this PR.

We should also cover the new Flat Config in v9 if it isn't covered already.

@Uzaaft
Copy link
Member

Uzaaft commented Sep 23, 2023

It's likely that using .eslintrc without a file ending is a legacy thing, pretty sure I've seen it before.

Just wanted to chime in and say that it is, and want to stand with merging of this PR.

We should also cover the new Flat Config in v9 if it isn't covered already.

Feel free to open a PR. :)

Also, I'm just waiting on @Dartv to fix the one thing I asked for before merging :)

@taskylizard
Copy link
Contributor

I was hoping that can be done in this PR anyway since it's just one file format: eslint.config.js

@Uzaaft
Copy link
Member

Uzaaft commented Sep 23, 2023

I was hoping that can be done in this PR anyway since it's just one file format: eslint.config.js

We prefer that PR's do one thing and one thing only. This one adds support for the legacy .eslintrc file, and that other PR will add support for another file. :)

@Dartv
Copy link
Author

Dartv commented Sep 25, 2023

@Uzaaft sorry, I'm not sure I follow. What thing do I need to fix?

@Uzaaft
Copy link
Member

Uzaaft commented Oct 8, 2023

I fixed it myself :) Opened a new PR to fix this issue.

@Uzaaft Uzaaft closed this Oct 8, 2023
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.

4 participants