Can't update @typescript-eslint

[joachimvh]

The latest version of @typescript-eslint/eslint-plugin throws an error when using the eslint-config-es settings. Reason being that the input that is allowed for a rule was changed. Apparently eslint-config-es does not update regularly so until that is fixed we should stay on the version we currently have. This issue is mostly to remind us about this should we get errors when updating all dependencies (and to check later on if it is fixed).

[joachimvh]

This also blocks the updating of all typescript/eslint related dependencies. (See #82 #91 #108 )

eslint-config-es seems to have Typescript 4 in its dependencies now though so might have to investigate further what the issues are for updating here.

The issue is also 1 specific line in the config so would be nice if we could somehow just exclude that one.

[joachimvh]

The good news is the config has been updated to remove the rule that was breaking the update so now we can update typescript/eslint/typescript-eslint/etc.

The "bad" news several new rules have been introduced which we're currently breaking. Since linting rules often lead to discussions I'll list the new ones we're breaking here together with my opinions on them so you can quickly say which ones you want to keep (so no 3 PRs are needed before everyone is happy 😉 )

@typescript-eslint/consistent-type-imports

Forces you to use import type ... instead of import when importing types/interfaces. Don't really care about this one either way since who ever looks at the imports list directly. Don't think this is relevant for us. (Apparently has some uses when you use babel).

@typescript-eslint/naming-convention

This changed to strictCamelCase which doesn't allow our variable webID since it has 2 following uppercase characters. I would just disable this since this seems like something we might do again in the future. It is a large block that would be disabled then though as seen here.

@typescript-eslint/lines-between-class-members

No reason to have this.

@typescript-eslint/no-unsafe-assignment

This one errors when assigning an any value to a variable that expects a specific type. I like this one as it enforces you to never use any when results are returned. Would disable it for the test files though (pretty sure we can do that at least?).

unicorn/catch-error-name

They changed the variable name they want you to use in catch blocks 🙄 . Used to be error but now it has to be ex.* for some reason. Would just disable.

@typescript-eslint/no-implicit-any-catch

I'm not sure I fully get this one (click the name for related issue discussing this), but apparently you can type errors you catch in the new typescript, and it also allows any and unknown which could muddy the typings. This rule enforces you to type the variable (but I'm not sure how you're supposed to be 100% sure what you're catching).

@typescript-eslint/unbound-method

This disallows the const { join: joinPath, normalize: normalizePath } = posix; we're currently using. It's to protect you from using class functions independently and thereby losing the this scoping. Think I would just disable this one.

unicorn/no-object-as-default-parameter

Prevents you from using an inline object as a default value for a function parameter as we do here. Would just disable.

unicorn/prefer-optional-catch-binding

Apparently you can have catch { now if you're not doing anything with the error. No reason not to keep this one.

[RubenVerborgh]

Agree with all your assessments, except that I'd be fine with webId.

[rubensworks]

Agree with all. See some notes below:

Forces you to use import type ... instead of import

I'd be fine with this rule. I've noticed some linter rules can even break if you don't follow this convention.

webID

I would also prefer webId actually :)

@typescript-eslint/lines-between-class-members

Probably has an auto-fixer, so wouldn't hurt to have.

@typescript-eslint/no-unsafe-assignment

+1 to only disable for test file. See Comunica repo on how to do that.

[RubenVerborgh]

Probably has an auto-fixer, so wouldn't hurt to have.

And it has "exceptAfterSingleLine": false as default, so good.

[joachimvh]

Probably has an auto-fixer, so wouldn't hurt to have.

And it has "exceptAfterSingleLine": false as default, so good.

My main issue with that one is how much screenspace that takes up when you have a few variables (also that default means that there should be no exceptions).

[RubenVerborgh]

Ah no agreed, I had misread it. I wouldn't want space between just variables.

[joachimvh]

Missed another part of the naming-convention one: all boolean functions/variables should start with is, has, etc. (as seen here). So that would also require changing the binary field to isBinary (and would require disabling that line when calling a library with an input options object that does not adhere to those rules...). I could just cut out that part and keep the camel case part of the rule that's in there.

[rubensworks]

all boolean functions/variables should start with is, has, etc. (as seen here).

Looks nice a nice convention to me :-)
I try to follow that myself most of the times.

and would require disabling that line when calling a library with an input options object that does not adhere to those rules...

That's a good reason against. However, that would probably also apply to some other rules.