fix(no-multiple-actions-in-effects): use type-checking to report for all known nodes

[rafaelss95]

Currently the rule only uses type-checking for explicit returns, but one could return a computed Array using a implicit return (ArrowFunctionExpression) or CallExpression.

[rafaelss95]

An interesting behavior I've noticed is that, this:

return actions.length > 0 ? actions : of(foo());

doesn't report as we may expect, but this:

return actions.length > 0 ? actions : null;

does.

Do you have any clue why does this happens? In any case I've added the tests to document this behavior.

---

That being said, to provide a full support for all possible cases like ?., ??, ? :, ||, && would increase the complexity and I'm not too sure it brings much value as no one complains so far.

[timdeschryver]

For now, let's keep it like this.
I think it's because the return type is Observable? Besides that, I wouldn't know the reason

The return type will be something like this, perhaps that the culprit is couldBeType here and that it doesn't play well with unions

Action[] | Observable<{
    type: string;
}>

[rafaelss95]

Yeah, I didn't dig too much into this, but maybe it's a problem with couldBeType.

In theory, couldBeType(node, 'Array') should return true if anything within the expression may return an Array, so it shouldn't return false if either side is Observable or something else, as the other side is actually an Array.

[timdeschryver]

@rafaelss95 fyi, I updated the comment... you're too fast for me 😂

[timdeschryver]

👀 it actually handles it correct - https://github.com/cartant/tsutils-etc/blob/master/source/could-be-type.ts#L30
The problem here, is that we should import of from rxjs.
Otherwise its type will just be any and thus isn't a union.

[rafaelss95]

I keep missing those imports within those strings 🤦

[timdeschryver]

You and me both...
Just a random thought, would it make sense to:

• create a seperate file for each case and divide them into a valid and invalid subfolder
• build the spec dynamically by reading those files and adding them as cases?

That way the individual file could be typechecked.

Caveats:

• probably slower, but not by much (I think)
• file need to be named and naming is hard. What about just using numbers and increment them? a comment as the first line in the file could comment what scenario is tested

[rafaelss95]

You mean something like what tslint used to do? I think it's a fantastic idea. Just to clarify: would we use this approach only for rules with type-checking or...?

[timdeschryver]

@rafaelss95 I don't think we'll be able to pull it of straight away.
I didn't think of the messageId and the rules that have an output 😅

[rafaelss95]

Yes, that's not trivial on a second thought 😆

[github-actions]

🎉 This PR is included in version 1.42.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀