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 #498

Closed
Js-Brecht opened this issue Feb 6, 2020 · 4 comments
Closed

.eslintrc doesn't override defaults #498

Js-Brecht opened this issue Feb 6, 2020 · 4 comments
Labels
scope: integration Related to an integration, not necessarily to core (but could influence core) solution: workaround available There is a workaround available for this issue

Comments

@Js-Brecht
Copy link

Current Behavior

My .eslintrc settings are ignored.

Expected behavior

If I define a .eslintrc in my repository, I expect its rules, and its rules alone, to be used. I do not ever, under any circumstances, want a dependency to set things that I cannot change, especially when it comes to linting/formatting my code.

Suggested solution(s)

Currently, settings from the package.json.eslint property will override any configurations during linting, but not the rules from .eslintrc. This is not ideal. I prefer to use .eslintrc or .eslintrc.js to define my eslint rules, since I find that it makes my life a little easier. Instead of having to go digging through my package.json to find my eslint rules any time I need to make changes, I can just go straight to where they are defined. package.json can already wind up with quite a bit of information in it, and eslint rules can also wind up being extensive. Easier to keep them segregated.

Not to mention, vscode-eslint doesn't care about package.json.eslint settings. So, if I want to make changes to my eslint, I now have to do it in two places.

If using .eslintrc is not a desired behavior, perhaps it might be better to let users opt out of tsdx's linting altogether? Mainly, the issue I have is during the build. When I need to lint my code, I can do it with eslint instead, which will use my rules as I have them configured.

Additional context

Other than that, I have to say, great tool! 👍

Your environment

Software Version(s)
TSDX 0.12.3
TypeScript 3.7.5
Browser n/a
npm/Yarn pnpm
Node 10.16.3
Operating System Windows 10
@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 7, 2020

TSDX does support .eslintrc etc, it's built-in when using ESLint's CLIEngine API (note that useEslintrc defaults to true). TSDX uses it here.

I also use tsdx lint with a .eslintrc.js in at least one of my packages (c.f. agilgur5/react-signature-canvas#42 ) and I believe that's why tsdx lint --write-file exists as well: to allow further configuration with a custom .eslintrc.js.

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. E.g. I use standard, so I have 'prettier/prettier' set to 'off' as they conflict with each other.
I'm not sure if there's a way to change this behavior with ESLint's API though, I looked into it as it was annoying/confusing me too.


Some separate things to respond to that aren't directly related:

I do not ever, under any circumstances, want a dependency to set things that I cannot change, especially when it comes to linting/formatting my code.

That is an opinion, plenty of opinionated libraries exist that don't allow you to change settings, like standard (which exists explicitly to not be changed to avoid bikeshedding).
TSDX is also an opinionated tool, but currently it does support customizing various things, and you can change the ESLint settings of tsdx lint, they just seem to be opt-out afaict per my above comments.

Not to mention, vscode-eslint doesn't care about package.json.eslint settings.

That sounds like a bug with vscode-eslint? It should behave the same as ESLint does, which is what TSDX does.

perhaps it might be better to let users opt out of tsdx's linting altogether?

You can certainly opt-out of these extras of TSDX -- you don't have to use tsdx lint and can just use eslint if you want (same with tsdx test vs. jest or any other test framework). That won't affect tsdx build.
TSDX has these built-in as a convenience (and adds some config for you), but you're not forced to use them if you don't want to.
I've also mentioned in other issues splitting off an eslint-plugin-tsdx (and babel preset and jest preset) or something for users who want to further customize their set-ups and still want to use parts of TSDX -- would definitely like to do that. That would also make --write-file output a one-liner like extends: ['plugin:tsdx'] or something

@Js-Brecht
Copy link
Author

Js-Brecht commented Feb 7, 2020

I do not ever, under any circumstances, want a dependency to set things that I cannot change, especially when it comes to linting/formatting my code.

That is an opinion, plenty of opinionated libraries exist that don't allow you to change settings, like standard (which exists explicitly to not be changed to avoid bikeshedding).

I want to apologize for how that came out. It was an overly dramatic presentation of my own opinion. I do get that a lot of libraries don't let you change their settings, and usually for good reason (Almost everything I've ever worked on/designed has at least a little bit of that baked in). I meant that to be very specifically about linting/formatting, and it came out much more generalized.

This is what happens when I get too little sleep 😞


you don't have to use tsdx lint and can just use eslint if you want (same with tsdx test vs. jest or any other test framework). That won't affect tsdx build.

What led me to filing this issue in the first place was that tsdx build was bombing out because of prettier. I never told it to lint anything.

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

That's the exact issue I was having
I was picturing a setup something like this, but perhaps there's good reason it wasn't done like this already? Maybe so that eslint could worry about merging configs, and the library could still maintain it's opinion, as it were?

       const cli = new CLIEngine({
         baseConfig: {
           ...config,
           ...appPackageJson.eslint,
+          .../* insert .eslintrc here */
         },

I suppose it would be easy enough to keep the extends in package.json and .eslintrc. It rarely changes. It's just the individual ruleset that can have some flux from time to time. It would be easier than switching rules on and off to try to compensate for rulesets that were set beforehand. Maybe even just use package.json to unset the extends property before .eslintrc gets merged in 🤔

E.g. I use standard, so I have 'prettier/prettier' set to 'off' as they conflict with each other.

The entire issue I had with the linting was because of the prettier plugin. Are you switching off every prettier rule, or is there now support for turning off an entire plugin's ruleset en masse? (see below) I remember a discussion a while ago about the desire for that feature.

  • I actually just noticed that eslint-plugin-prettier usage is a single rule; which is what you were talking about prettier/prettier = "off". This is how little I use prettier 😆

I've also mentioned in other issues splitting off an eslint-plugin-tsdx (and babel preset and jest preset) or something for users who want to further customize their set-ups and still want to use parts of TSDX

That sounds like it would be ideal.

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 7, 2020

I meant that to be very specifically about linting/formatting, and it came out much more generalized.

No worries. Though the example I put of standard is linting/formatting-specific, and it doesn't normally let you change settings. Another notable example is gofmt, terraform format etc

What led me to filing this issue in the first place was that tsdx build was bombing out because of prettier. I never told it to lint anything.

Yea it kinda sounded like that's what you meant, but that's very confusing because tsdx build definitely doesn't call anything related to ESLint or Prettier. #407 even makes that split in the source code very explicit.
Do you have a repro? Maybe you're adding a rollup plugin that checks linting during build?

I was picturing a setup something like this, but perhaps there's good reason it wasn't done like this already? Maybe so that eslint could worry about merging configs, and the library could still maintain it's opinion, as it were?

I looked into this before (like days ago but my head is very full this past week) and IIRC this was a common set-up. I'm pretty sure that ESLint does some more complex resolution than simply reading the file (e.g. extends etc) but idk for sure. Had some similar issues with tsconfig parsing pop up recently (c.f. #489 ). Definitely better to let the tool parse its own configs whenever possible and avoid anything custom as that can quickly become a rabbit hole

  • which is what you were talking about prettier/prettier = "off"

To avoid any confusion, it's:

module.exports = {
  rules: {
    'prettier/prettier': 'off' // override tsdx lint
  }
}

@Js-Brecht
Copy link
Author

Okay, now I'm confused. I had wiped out what I was doing previously, so I went to recreate it and share the results with you... but I'm having trouble doing that. I can't seem to replicate the issue any longer 🤷‍♂️.

Maybe it was an environment issue. I'm not using the same machine now as I was then. Later next week I'll try to recreate the issue on the other machine.

I'll go ahead and close this. If I'm able to recreate the issue, I'll post back here.

@agilgur5 agilgur5 changed the title User defined .eslintrc settings are not used during building/linting .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 9, 2020
This was referenced Mar 17, 2020
@agilgur5 agilgur5 added the solution: workaround available There is a workaround available for this issue label Oct 16, 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: workaround available There is a workaround available for this issue
Projects
None yet
Development

No branches or pull requests

2 participants