refactor: use `getNgRx*` to grab all `Store` instances #221

rafaelss95 · 2021-09-20T01:34:09Z

So, I've opened this PR to discuss in practice what something we discussed some time ago #189 (comment)...

The proposal here is to normalize the store retrieval. Instead of getting just the string that represents the Store, we get the Identifier, or rather, not just the first one, but all of them (Identifier[]), basically findNgRx* (simple string) would become getNgrx*Identifiers (Identifier[]).

With this we'll be able to use this method for the remaining rules that still don't use this method to grab the store(s): no-multiple-global-stores, no-typed-global-store and use-consistent-global-store-name.

Question/Statements:

Currently, the vast majority of the rules, except the three mentioned above only cares about the first injected Store. Should we apply this to no-typed-global-store and use-consistent-global-store-name too? Or should we change the behavior of no-typed-global-store and use-consistent-global-store-name to report only the first match?

You can see use-selector-for-select's changes how it would look like if we apply the option 1 => it basically loop through the stores and build the visitors using reduce) and you can also see no-typed-global-store's changes how it would look like if we apply the option 2 => it basically gets the first injected instance as we do in the other rules).

If you ask me, I don't have a strong opinion about this, but for me, if you have more than one injected Store you either injected by a mistake or I don't know what the reason could be 😅

Maybe there's a case I didn't think you need to inject more than one? 🤔

In any case, let me know your thoughts.

Please, note that this is just a draft and not necessarily must be merged.

rafaelss95 · 2021-09-20T01:34:39Z

src/utils/helper-functions/utils.ts

+  }, [])
+}
+
+export function getNgRxStoreIdentifiers(


or maybe

Suggested change

export function getNgRxStoreIdentifiers(

export function getNgRxStores(

`getNgRxStoreIdentifiers is fine for me

rafaelss95 · 2021-09-20T01:35:08Z

src/rules/store/use-selector-in-select.ts

+    return stores.reduce(
+      (visitors, store) => ({
+        ...visitors,
+        [`${pipeableSelect(store.name)}, ${storeSelect(store.name)}`](
+          node: TSESTree.CallExpression,
+        ) {


This would grab any existing Store.

rafaelss95 · 2021-09-20T01:35:28Z

src/rules/store/use-consistent-global-store-name.ts

-        typeAnnotation: TSESTree.TSTypeAnnotation
-      }) {
+      Program() {
+        const [store] = getNgRxStoreIdentifiers(context) ?? []


Only the first Store would be reported.

I changed this behavior and we're now reporting all stores

timdeschryver

I like the idea.
Having multiple stores is a bad practice (that's why it's also a rule).
For our rules, I think it makes sense to report all the stores (because the multiple stores rule could be turned off).

Did you check the performance gains? or is this just a gut feeling?

If you can resolve the conflicts, I think that we can merge it

timdeschryver

Feel free to merge this @rafaelss95 if it's ok for you.

rafaelss95 · 2021-10-07T04:16:50Z

@timdeschryver I prefer to change a bit before merging. I opened it with little change as possible, so you could give me ur opinion. Now that I have your opinion validated, maybe I could change some other rules already in this PR. WDYT? If so, I can do it tomorrow when I get some more time. I've been busy these days 😞

timdeschryver · 2021-10-07T10:00:32Z

Hi @rafaelss95 don't worry about it.
You can refactor existing rules in this PR if you want, just make sure that you commit per rule as that would be easier to review for me (but I see you're already doing that 👍).
Ping me when the PR is ready to be reviewed.

…w way + tests

rafaelss95 · 2021-10-11T01:52:46Z

@timdeschryver Done! I think I finished everything I had planned here.

The PR got huge mostly due to the testing changes, but I tried to separate as much as I could. In any case, let me clear up any doubts... and I'm totally open to breaking it if you feel more comfortable.

Also, please take as much time as you think necessary to review it.

Btw, I didn't touch the avoid-cyclic-effects tests to avoid conflicts with #254 (but I still plan to add tests to cover multiple Actions) in another PR.

timdeschryver · 2021-10-11T17:36:00Z

Hi @rafaelss95 after a first glance it looks super!
I'll go through it in more details later this week.
Just one question, I'm assuming you didn't make changes to the contents of a tests, just the formatting. Is that assumption valid?

rafaelss95 · 2021-10-12T15:07:29Z

I'm assuming you didn't make changes to the contents of a tests, just the formatting. Is that assumption valid?

Not really. Besides that, there are some new tests to ensure we cover multiple Store-like and also some naming changes for consistency (like class somethingElse to class Ok<index> / NotOk<index> (to help in debugging - and also to maintain a standard).

timdeschryver

Looking good! I have nothing else to say 🚀

github-actions · 2021-10-12T17:37:01Z

🎉 This PR is included in version 1.46.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

rafaelss95 requested a review from timdeschryver September 20, 2021 01:34

rafaelss95 commented Sep 20, 2021

View reviewed changes

github-actions bot added the conflicts There is a merge conflict label Sep 25, 2021

rafaelss95 mentioned this pull request Oct 4, 2021

fix(prefer-action-creator-in-dispatch): properly handle actions with literal objects #256

Merged

timdeschryver reviewed Oct 4, 2021

View reviewed changes

github-actions bot removed the conflicts There is a merge conflict label Oct 5, 2021

timdeschryver marked this pull request as ready for review October 5, 2021 17:00

timdeschryver approved these changes Oct 5, 2021

View reviewed changes

timdeschryver mentioned this pull request Oct 7, 2021

test: use uvu to run rule tests for better reports #257

Merged

rafaelss95 force-pushed the feat/proposal-use-method-to-grab-stores branch from 0508e76 to ed635e1 Compare October 10, 2021 20:20

rafaelss95 added 16 commits October 10, 2021 22:30

refactor: rename selectors and functions

74b484f

refactor(updater-explicit-return-type): use the new way + tests

ef5787b

refactor(avoid-cyclic-effects): use the new way

5b4c0fb

refactor(no-dispatch-in-effects): use the new way + tests

6f966ea

refactor(avoid-combining-selectors): use the new way + tests

9d76f01

refactor(avoid-dispatching-multiple-actions-sequentially): use the ne…

adc42a5

…w way + tests

refactor(avoid-mapping-selectors): use the new way + tests

869d36d

refactor(no-multiple-global-stores): use the new way + tests

fe52874

refactor(no-store-subscription): use the new way + tests

df54063

refactor(no-typed-global-store): use the new way + tests

0dd6e40

refactor(prefer-action-creator-in-dispatch): use the new way + tests

c2670fe

refactor(use-selector-in-selector): use the new way + tests

9abd058

refactor(select-style): use the new way + tests

a741c21

refactor(prefer-concat-latest-from): use the new way + tests

b61ccb2

refactor(use-consistent-global-store): use the new way + tests

5f3c081

build: enable curly rule

93e8432

rafaelss95 force-pushed the feat/proposal-use-method-to-grab-stores branch from ed635e1 to 93e8432 Compare October 11, 2021 01:30

timdeschryver approved these changes Oct 12, 2021

View reviewed changes

timdeschryver merged commit c392618 into timdeschryver:main Oct 12, 2021

github-actions bot added the released label Oct 12, 2021

rafaelss95 deleted the feat/proposal-use-method-to-grab-stores branch October 13, 2021 00:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use `getNgRx*` to grab all `Store` instances #221

refactor: use `getNgRx*` to grab all `Store` instances #221

rafaelss95 commented Sep 20, 2021 •

edited

Loading

rafaelss95 Sep 20, 2021

timdeschryver Oct 4, 2021

rafaelss95 Sep 20, 2021

rafaelss95 Sep 20, 2021

timdeschryver Oct 5, 2021

timdeschryver left a comment

timdeschryver left a comment

rafaelss95 commented Oct 7, 2021

timdeschryver commented Oct 7, 2021

rafaelss95 commented Oct 11, 2021

timdeschryver commented Oct 11, 2021

rafaelss95 commented Oct 12, 2021

timdeschryver left a comment

github-actions bot commented Oct 12, 2021

	export function getNgRxStoreIdentifiers(
	export function getNgRxStores(

refactor: use getNgRx* to grab all Store instances #221

refactor: use getNgRx* to grab all Store instances #221

Conversation

rafaelss95 commented Sep 20, 2021 • edited Loading

rafaelss95 Sep 20, 2021

Choose a reason for hiding this comment

timdeschryver Oct 4, 2021

Choose a reason for hiding this comment

rafaelss95 Sep 20, 2021

Choose a reason for hiding this comment

rafaelss95 Sep 20, 2021

Choose a reason for hiding this comment

timdeschryver Oct 5, 2021

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

rafaelss95 commented Oct 7, 2021

timdeschryver commented Oct 7, 2021

rafaelss95 commented Oct 11, 2021

timdeschryver commented Oct 11, 2021

rafaelss95 commented Oct 12, 2021

timdeschryver left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2021

refactor: use `getNgRx*` to grab all `Store` instances #221

refactor: use `getNgRx*` to grab all `Store` instances #221

rafaelss95 commented Sep 20, 2021 •

edited

Loading