eslint-config-hardcore icon indicating copy to clipboard operation
eslint-config-hardcore copied to clipboard

Separate `ts-for-js` and `ts`

Open matwilko opened this issue 11 months ago • 2 comments

Currently, importing ts.json also imports ts-for-js.json. This changes a lot of things at a global level, assuming that the user has kept the ESLint default of linting all .js files.

However, in my project, I made .ts files the default, and explicitly ban .js files, so when I import ts.json and get all the ts-for-js rules along for the ride on my .ts, it makes things much messier!

Given that ts-for-js is listed separately on the README.md, this behaviour was unexpected, as my expectation was that if I had both .ts and .js files, I would need to import both configs myself.

I'd like to suggest that ts.json remove the ts-for-js.json import, and make it clear to users that they will need to import both if they have both .ts and .js files they want to lint with TypeScript.

I know that's a breaking change, so as a non-breaking, but potentially more compatible alternative, could ts-for-js.json be updated to use { overrides: { files: ["**/*.js"], rules: { ... } }?

matwilko avatar Jul 21 '23 16:07 matwilko

hardcore/ts is intended to be a superset of hardcore/ts-for-js, i. e. hardcore/ts is intended to include all rules from hardcore/ts-for-js, plus TypeScript-specific rules.

Could you explain how it makes things much messier?

EvgenyOrekhov avatar Jul 21 '23 22:07 EvgenyOrekhov

Sure, from my perspective, when I explicitly pulled in ts.json, my expectation was that I was enabling just Typescript support from hardcore. The fact that JS stuff came along for the ride was surprising when there's a separate config to enable that, and my expectation was that JS-specific stuff wouldn't be enabled unless I pulled in that config as well because the README states "additional config for linting JavaScript".

The messiness comes from the fact that ts-for-js is globally scoped (i.e. applies to all files), and and so when I saw that it was being included, I had to spend some time looking through to see what it was applying to Typescript files as well as JS files, and I quickly gave up trying to reconcile ts-for-js and ts, and patched ts.json to remove the ts-for-js.json dependency, thinking it was only touching rules intended for JS files, and safe to simply exclude.

If ts is intended as a superset of ts-for-js, then I think this either needs making explicit in the README, or the rules in ts-for-js need to be merged into ts to properly separate the two configs, and make sure that ts-for-js is actually an additional config just for JS files.

In any case, I believe ts-for-js absolutely needs to be modified to use an override rather than being global. The README already recommends using an override scoping its use to *.js files.

This is because the lack of scoping is going to start causing bugs when the config is updated to the new Flat Config system - because ts-for-js is globally scoped, it will override ts if it is included in the flat config after ts, that is:

// Speculative names from future version supporting flat config
import { ts, tsForJs } from 'eslint-config-hardcore';

export default [
    ts,
    tsForJs // a user might do this because they assume that ts-for-js is a separate config they need to include for JS files!
];

Would result in ts-for-js overriding ts's different configuration for .ts files for any rules that they share in common. And it's not unreasonable for a user to think the above is a valid configuration, given the documented usage.

I think the fix for this is to completely separate the two configs so that they can be included in any order in a flat config (because they have different files: [...] specifications).

So, overall, I think the ask here is that either any interdependencies/relationships between the configs are made explicit in the README so users know what they're getting, and/or make the configs truly independent and live up to their documented use, so that when I include one, I'm getting just what I intended to include, and the configs are all additive and non-conflicting.

matwilko avatar Jul 24 '23 10:07 matwilko