Add more fixers/suggestions

[rafaelss95]

Information about the new behavior

Thinking about a better DX, we could add fixers/suggestions for some more rules.

Rule	Fixer	Suggestions	PR #
avoid-duplicate-actions-in-reducer	❌	✅	#209
no-dispatch-in-effects	❌	✅	#211
no-effect-decorator-and-creator	✅	⚠️ (if the @Effect contains arguments, the fixer would not be safe.)	#191
no-effects-in-providers	Probably ✅ (can this cause any unexpected behavior?)	❌	#203
no-multiple-global-stores	❌	✅	#198
no-reducer-in-key-names	❌	✅	#200
on-function-explicit-return-type	❌	⚠️ (we can suggest State as it's a common convention in reducer files)	#199
prefix-selectors-with-select	❌	✅	#205
select-style	✅	❌	#189
use-consistent-global-store-name	❌	✅	#197

Let me know your thoughts on this, @timdeschryver :)

I would be willing to submit a PR for the docs ❤️

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[timdeschryver]

I would like to know what the fixer/suggestion would do 😅
Also, the @effect decorator will be removed soon - so this is also a low prio for me.

Am I correct to assume that the fixer/suggestion would do the following?
no-dispatch-in-effects: remove the dispatch
no-effects-in-providers: remove the provider
no-multiple-global-stores: remove one store, what about the references to that store?
no-reducer-in-key-names: this might be tricky, because it might impact other code (e.g. selectors) - so I would prefer to not have a fixer here
on-function-explicit-return-type: I think that State is used in a lot of example, but in real world applications I see a lot of "named" states, e.g. PersonsState. If we could infer the State interface that would be great, but I'm not sure if this is doable
select-style: 👍👍 I needed this the other week 😅
use-consistent-global-store-name: here again, we should also update all references that use the store variable

[rafaelss95]

no-dispatch-in-effects: remove the dispatch

✅

no-effects-in-providers: remove the provider

✅

no-multiple-global-stores: remove one store, what about the references to that store?

As it's a suggestion, I don't think we should try to change all references because that can be quite problematic as people might be using it in template, in a subclass or even in tests 🤷🏿.

no-reducer-in-key-names: this might be tricky, because it might impact other code (e.g. selectors) - so I would prefer to not have a fixer here

I actually suggested it as a suggestion, so it would be up to developers whether or not to follow the tip 😅

on-function-explicit-return-type: I think that State is used in a lot of example, but in real world applications I see a lot of "named" states, e.g. PersonsState. If we could infer the State interface that would be great, but I'm not sure if this is doable

Same as above for no-reducer-in-key-names.

select-style: 👍👍 I needed this the other week 😅

✅

use-consistent-global-store-name: here again, we should also update all references that use the store variable

Same as no-multiple-global-stores.

[timdeschryver]

So, if I understand correctly a suggestion could be used as: "hey, this is how it would look like - it's up to you to fix related problems"?

[rafaelss95]

So, if I understand correctly a suggestion could be used as: "hey, this is how it would look like - it's up to you to fix related problems"?

Yup, that's basically my point of view on the suggestions topic. IMHO, providing suggestions can serve both as a help for the developer to resolve this report (not necessarily fixing all related issues) — see @angular-eslint/template/no-any/cases, but also as an aid in case there are multiple ways to resolve a report and only one fix would not be the best option — see @angular-eslint/use-injectable-provided-in/cases.

As they're manually applied, there's no risk involved as people have to see the suggestion and confirm that's what they want, different, of course, of a fixer, that could be automatically applied in multiple files at once using a script, for example.

---

And another closer example... here in the repo, we offer suggestions for prefer-inline-action-props:

eslint-plugin-ngrx/tests/rules/prefer-inline-action-props.test.ts

Lines 21 to 34 in d3a645c

	{
	code: `const notOk0 = createAction('notOk0', props<number>())`,
	errors: [
	{
	messageId: preferInlineActionProps,
	suggestions: [
	{
	messageId: preferInlineActionPropsSuggest,
	output: `const notOk0 = createAction('notOk0', props<{name: number}>())`,
	},
	],
	},
	],
	},

It certainly breaks the places where you are using notOk0(1). So you need to replace manually it to notOk1({name: 1}).

[timdeschryver]

Thanks @rafaelss95

[rafaelss95]

@timdeschryver:

1. about no-dispatch-in-effects... since feat: add fixer for no-dispatch-in-effects #73 was closed (I confess I didn't even remember that), I guess it's not appropriate to send a PR with these changes, am I right?
2. what do you think about adding suggestion for avoid-duplicate-actions-in-reducer too?

---

Not really related to this issue, but what would be the difference between prefixing some rules with "avoid-" and others with "no-"? I always had this curiosity 😅

[timdeschryver]

@rafaelss95 yea, with the recently added suggestions (and your comment #179 (comment)) I kind of see it different now. I think adding the suggestion would be good 👍

avoid-duplicate-actions-in-reducer, yes that's fine by me. Would it remove the selected line, or the other violations of the rule?

I thought of avoid- as a guideline (a warn) e.g. , while no is a best practice (an error).

[rafaelss95]

@rafaelss95 yea, with the recently added suggestions (and your comment #179 (comment)) I kind of see it different now. I think adding the suggestion would be good 👍

✅

avoid-duplicate-actions-in-reducer, yes that's fine by me. Would it remove the selected line, or the other violations of the rule?

It would remove the whole CallExpression, for example:

const reducer = createReducer(
  on(foo1, state => state),
  on(foo1, ({bar}) => bar), // if you click on it, it would remove this whole line, otherwise the other(s).
)

I thought of avoid- as a guideline (a warn) e.g. , while no is a best practice (an error).

It makes perfect sense! Thanks for clarifying :)

[rafaelss95]

I was looking at existing suggestions and was curious to see if there is any specific reason why prefer-concat-latest-from offers a suggestion and not a direct fix...

[timdeschryver]

@rafaelss95 that's indeed a good question! I can't remember why we did it like that, it can be converted to a fix for me 👍

[rafaelss95]

The funny part is that it's already marked as fixable 😅:

eslint-plugin-ngrx/src/rules/effects/prefer-concat-latest-from.ts

Line 28 in dc1ff4a

fixable: 'code',

^{The same is incorrect for no-typed-global-store}

---

Btw, the following case is currently producing compilation error:

- withLatestFrom(this.store$.select(something)),
+ concatLatestFrom(this.store$.select(something)), // TS Error
+ concatLatestFrom(() => this.store$.select(something)), // This should be the correct

But in general yes, I think we could offer a fixer for most cases, except for this rare case, which would be kept as a suggestion:

withLatestFrom(this.store$.select(something), (one, other) => somethingElse()),

---

Maybe a dumb question... but I see that we have this case using withLatestFrom intentionally in the valid section:

eslint-plugin-ngrx/tests/rules/prefer-concat-latest-from.test.ts

Lines 27 to 38 in fb35ca2

	class Effect {
	detail$ = createEffect(() => {
	return this.actions.pipe(
	ofType(ProductDetailPage.loaded),
	concatMap((action) =>
	of(action).pipe(withLatestFrom(this.store.select(selectProducts))),
	),
	mergeMap(([action, products]) => {
	})
	)
	})
	}

Is it because it's already considered a "lazy fetch"?

[timdeschryver]

😅 I think that the suggestion doesn't break things (e.g. concatLatestFrom(this.store$.select(something)))?
Also, keep in mind that concatLatestFrom(action => this.store$.select(action .something)) is also valid.
For the overload with the projector, we can leave it as a suggestion, or we can make it even smarter (perhaps that this will be done in a RxJS linter/migration because that syntax is now deprecated IIRC).

The projector can be replaced with a map, so this would become

source$.pipe(withLatestFrom(this.store$.select(something)), map(([one, other]) => somethingElse()))

For the "dumb" question (which isn't dumb 😉), the idea was that it was lazy so it could be left as is.
In practice, this is probably something that you want to also have refactored with concatLatestFrom.
At least, we did it manually in our project...

[rafaelss95]

For this lase case, perhaps we could provide an option (something like strict 🤔) so people can decide if they want to maintain concatMap(action => of(action).pipe(withLatestFrom(something))) version even if it would be shorter with concatLatestFrom(() => this.select(something)?

[timdeschryver]

I like the config option 👍

[github-actions]

🎉 This issue has been resolved in version 1.46.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀