[eslint-plugin-mobx] mobx/exhaustive-make-observable autofix behavior should be inversed

[kade-robertson]

Intended outcome:

As per the documentation of makeObservable, it is expected that if you do not currently have an annotation for a member, it is not affected.

I would expect that an autofix for mobx/exhaustive-make-observable would put a value for all missing members that should be listed, but have the annotation set to false to maintain the behaviour from before. This seems especially weird because the warning message itself tells you to mark fields as false: "Missing annotation for <members>. To exclude a field, use false as annotation"

Actual outcome:

The mobx/exhaustive-make-observable autofix annotates all missing members with true, changing the behaviour of those members.

How to reproduce the issue:

You can fork this Devbox, run pnpm lint to see the warning with an autofix option, and pnpm lint:fix to see what the autofix does.

https://codesandbox.io/p/devbox/h7st47?file=%2Findex.js%3A14%2C20

Versions

eslint-plugin-mobx v0.0.9.

[urugator]

The biggest problem with makeObservable is that it's easy to forget making something observable. That's what this eslint rule solves. The autofix is intended to provide the conviniency of makeAutoObservable.

"Missing annotation for . To exclude a field, use false as annotation"

It' suspposed to say: Hey, you probably forgot to annotate a field, but if this is actually intended and you don't want to make this field observable, you don't have to disable this rule, you can just use false as annotation.

If you're makeObservable user, it's not immediately obvious you can use false as annotation, because it does nothing except for silencing this rule. Therefore the hint.

[kade-robertson]

I agree the eslint rule solves the problem it sets out to solve. I disagree with the autofix. You have no way of knowing what the intent of the missing field is, just that the user probably didn't mean it. But they very well might have!

In general, it feels bad to me to suggest an autofix that might lead the person to just change what was just fixed again, because you made a bad assumption about their intents. It seems to me like in these cases an autofix just shouldn't exist. Make them fix it themselves or turn it off (with the consequences that come with it). From what I can tell this is generally how autofixes are implemented -- they only apply in scenarios that are indisputably wrong AND it doesn't change the meaning of the code. I scrolled through all of the stock ESLint rules briefly and I don't think there is a rule where an autofix is present in an unambiguous situation like this. To be clear, I do realize my fix (defaulting to false) only deals with one of those branches -- it didn't change the meaning of the code but it still might not be what the user expects.

There may be a partial solution in keeping the autofix with some addditional configuration as to what the autofix does (the default value it fills out), but that's also not entirely free from ambiguity. Does that seem reasonable (in lieu of just not having an autofix)?

--

This is an aside, but if it's so easy to make a mistake with the current makeObservable that suggests to me the API should be more strict and enforce that all members have to be mentioned. I realize the friction that introduces but if this partial observability case is so unlikely you shouldn't be able to mistakenly do it at all -- maybe instead have a function makePartialObservable or just rely on decorators for this kind of functionality.

[urugator]

because you made a bad assumption about their intents

I understand the concern. I think false as a default makes sense if you enable the rule for an existing code.
But if you use it from the get go, you can't have any intentionally missings annotations. Missing annotation is a missing intent - the rule forbids that - it forces you to be explicit about your intents. In such case, 99% violations of this rule will be due to missing observable/action/.... Therefore true is a lot more sensible and useful default.

the API should be more strict and enforce that all members have to be mentioned.

It's not technically possible (for similar reasons the makeAutoObservable doesn't support inheritance).
It has non-negligible perf cost.
You would still have to write it manually.

Does that seem reasonable

I am ok with making the autofix configurable.
I am also fine with providing multiple fix suggestions if that's possible (last time I tried, ~3 years ago, it didn't work well in VScode).
true should stay default.
Docs should instruct users to first run eslint --no-eslintrc --fix --rule='mobx/exhaustive-make-observable: [2, { "autofixAnnotation": false }]' . if their code base already contains makeObservable calls and they want to preserve existing behavior (even if possibly invalid).

[kade-robertson]

It's not technically possible (for similar reasons the makeAutoObservable doesn't support inheritance).

Ah, okay. When I was thinking about this I was more thinking about having this enforced with TypeScript only, but that does neglect anyone not using it.

I am ok with making the autofix configurable.

Sounds good! I will close off my existing PR and get an updated one ready soon.

I am also fine with providing multiple fix suggestions if that's possible (last time I tried, ~3 years ago, it didn't work well in VScode).

This was my experience as well. I'll investigate and see if it's doable.

true should stay default.

Docs should instruct users to first run eslint --no-eslintrc --fix --rule='mobx/exhaustive-make-observable: [2, { "autofixAnnotation": false }]' . if their code base already contains makeObservable calls and they want to preserve existing behavior (even if possibly invalid).

✅