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

.eslintrc doesn't override defaults #514

Open
gamtiq opened this issue Feb 22, 2020 · 7 comments
Open

.eslintrc doesn't override defaults #514

gamtiq opened this issue Feb 22, 2020 · 7 comments
Labels
scope: integration Related to an integration, not necessarily to core (but could influence core) solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue

Comments

@gamtiq
Copy link

gamtiq commented Feb 22, 2020

First of all, thank you for the great tool!

Current Behavior

tsdx ignores .eslintrc.js or .eslintrc.json in project's root directory and uses default config when lint script is run.
At the moment it is possible to redefine default linting config only by specifying necessary settings in eslint key inside package.json. It is quite inconvenient.

Desired Behavior

Use .eslintrc.js or .eslintrc.json when it is present in project's root directory to redefine/replace default config.

Suggested Solution

Check presence of .eslintrc.js or .eslintrc.json in project's root directory and use its contents instead of default config returned by createEslintConfig.

Who does this impact? Who is this for?

All users wanting to customize linting settings.

Describe alternatives you've considered

Additional context

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 22, 2020

Duplicate of #498 . Per my comments there, tsdx lint does read .eslintrc ~6 lines below the code you linked. This is done by the ESLint API itself.

I use an .eslintrc.js with TSDX latest in my own projects and can't reproduce this. Please provide a repro.

@gamtiq
Copy link
Author

gamtiq commented Feb 23, 2020

Sorry, I missed #498 (I searched only for open issues).
I've added the following snippet in .eslintrc.js

    rules: {
        'prettier/prettier': 'off'
    }

as mentioned in the comment for 498 and fixed the problem.

By the way, I suppose the info from this comment (about You can change everything, but one difference with using an eslintrc file is that the internal rules will be added to any custom ones, unless you've specifically set them to off in your rules.) and this comment (adding 'prettier/prettier': 'off' to rules) should be added into README.md to avoid confusion.

@agilgur5
Copy link
Collaborator

Yea as this has popped up at least twice now, docs around this behavior would be helpful.

The behavior does feel unintuitive (I was confused by this initially too), but I'm not sure it can be changed without being breaking.
Similarly, as the ESLint API is handling loading .eslintrc, it's not easy to differentiate between the case of no .eslintrc (use defaults) and yes .eslintrc (don't use defaults, breaking change) 😕

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 27, 2020

Ahhh so I just realized why this didn't pop up with package.json#eslint -- that config is shallow merged with the default config, and all the extends are another level deep, so the whole extends array gets overwritten.

I guess the ESLint API does a deeper merge? This mismatched behavior is definitely something that should be fixed so that they work the same way... Unfortunately that has potential for breakage regardless of which one is changed, welp 😕

I'd lean toward matching the ESLint API, though I have a feeling it might automatically check package.json.eslint too, need to investigate more.
But if one has to break anyway, might make sense to get rid of the confusing baseConfig when using a custom eslintrc. Will have to see if the API has a way to detect custom options outside of CLIEngine.
If we remove baseConfig when a config is specified for tsdx lint, we should probably do the same with tsdx test for consistency. Still need presets and --write-file to make that easier though 😕 . That'd be double breaking then ooof

@gamtiq
Copy link
Author

gamtiq commented Feb 28, 2020

Imho baseConfig (or at least default config) should be ignored when there is an ESLint config file in project's root directory.
Another way is to add an option to skip/ignore default config.

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 28, 2020

Yea I think that's the less confusing option, particularly because of the editor integration and how nuanced linting tends to be (including me, you're at least the third to be confused by this).
The base Jest config merging doesn't seem to have been as confusing, though has raised issues with editor integrations as well.
The Babel config merging also hasn't been confusing (aside from some bugs)

As I alluded to here and explicitly mentioned in #498 , the difficulty with this will be determining when there is and isn't a custom config present. As there are like 4+ different extensions and package.json.eslint, it'd be a lot easier if the ESLint API handled it, but idk that it does.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 3, 2020

it'd be a lot easier if the ESLint API handled it, but idk that it does.

Seems like there might be a way to hack it together with CLIEngine#getConfigForFile, but there isn't really an exact API to check for existence of config, so I might have to file an issue with ESLint

@agilgur5 agilgur5 added the solution: duplicate This issue or pull request already exists label Mar 9, 2020
@agilgur5 agilgur5 changed the title Use .eslintrc for linting when it is present .eslintrc doesn't override defaults Mar 9, 2020
@agilgur5 agilgur5 added the scope: integration Related to an integration, not necessarily to core (but could influence core) label Mar 11, 2020
@agilgur5 agilgur5 added the solution: workaround available There is a workaround available for this issue label Oct 16, 2020
@agilgur5 agilgur5 added this to the Future Breaking milestone Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: integration Related to an integration, not necessarily to core (but could influence core) solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue
Projects
None yet
Development

No branches or pull requests

2 participants