Fix/lint constructor overloads

[ahoisl]

Fix the 'missing-make-observable' rule for overloaded constructors. The rule would always take the first ctor even if that is just the definition.

Code change checklist

• Added/updated unit tests
• Updated /docs. For new functionality, at least API.md should be updated
• Verified that there is no significant performance drop (yarn mobx test:performance)

[changeset-bot]

🦋 Changeset detected

Latest commit: 3c1642c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
eslint-plugin-mobx	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ahoisl]

@urugator the linting rules work well, thank you! I have just disabled the 'exhaustive-make-observable' and 'missing-observer' rules because they generate too many false positive warnings...

[urugator]

Aha, so that's what this TSEmptyBodyFunctionExpression is good for :D completely missed that... Thank you. Now I am a bit worried whether overloads are handled properly in other rules.

exhaustive-make-observable could you elaborate a bit? Keep in mind the intended way to exclude a field is to exclude it explicitely via false annotation. Should be reasonable in most cases I think.

missing-observer yea unfortunately as expected... I assume it's about functions - may I ask what kind of non-component functions it clashes with? Just want to get a picture if it's more about specific/uncommon naming convetions or if it's something obvious like pre-class syntax prototypes.
I would suggest observableRequiresReaction as a runtime alternative, but it also has it's downsides.

[ahoisl]

Regarding exhaustive-make-observable: It's complaining about fields and properties we explicitly don't want/need observable/computed. And we cannot use annotations because we're using decorators, remember? That was the problem in another issue 😉 I don't think there is a observable.not decorator

Regarding missing-observer: We have our own base component library that is made of controlled components not using mobx at all. The rule complains about each of those components.
Additionally, it warns about class mixin functions, i.e. functions that create/return a class (also not mobx related).

We already use observableRequiresReaction in development environments and it does its job nicely.

[urugator]

I don't think there is a observable.not decorator

That shouldn't be needed, because the rule should do nothing if you use decorators:
https://github.com/mobxjs/mobx/blob/main/packages/eslint-plugin-mobx/src/exhaustive-make-observable.js#L69
The only problematic case I can think of, would be if you have decorated super and you call makeObservable(this) in subclass without decorating anything - this shouldn't be normally possible, because makeObservable(this) throws/warns if there are no decorators, but the check probably fails here, because it sees decorators from super.
In other words if your (sub)class doesn't have any decorated fields it should't call makeObservable(this). At the same time you won't ever forget calling makeObservable(this), because of missing-make-observable rule.
So the rule assumes that if there is makeObservable(this), then there must be at least one decorator (in the same class, where it's called) or annotations must be provided.

To summarize:
exhaustive-make-observable is intended only for annotation users, but shouldn't affect decorator users.
missing-make-observable is intended only for decorator users, but shouldn't affect annotation users.

our own base component library

I guess this could be solved by the override as suggested in readme.

warns about class mixin functions

This one is more about naming convention I think. Maybe we could add an option to require some JSX inside the function, still not super reliable though.

Thank you for the feedback.

[ahoisl]

exhaustive-make-observable is intended only for annotation users, but shouldn't affect decorator users.

You are absolutely right, I interpreted the results in a wrong way and for a wrong component, sorry. It complained about a class that actually shouldn't have called makeObservable at all. So this rule seems fine, thanks!

I guess this could be solved by the override as suggested in readme.

You mean just enabling the rule on a subset of files? Could be true, but we also have a small controlled components that we do not apply observer on anywhere in code. So I guess we will rely on the runtime warnings instead, they work fine.