Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename getMockStore() to createMockStore() #3781

Closed
1 of 2 tasks
zbarbuto opened this issue Feb 22, 2023 · 5 comments · Fixed by #3789
Closed
1 of 2 tasks

Rename getMockStore() to createMockStore() #3781

zbarbuto opened this issue Feb 22, 2023 · 5 comments · Fixed by #3789
Labels
Accepting PRs community watch Someone from the community is working this issue/PR Good First Issue Good issue for first-time contributor Project: Store

Comments

@zbarbuto
Copy link
Contributor

zbarbuto commented Feb 22, 2023

Which @ngrx/* package(s) are relevant/related to the feature request?

store

Information

I have fallen into the trap a couple of times of thinking that getMockStore() was an alias for TestBed.inject(MockStore). For the most part, it will serve a similar purpose, and it the name hints that it will get the mock store I need.

However, it falls apart when you start to use things like overrideSelector as getMockStore().overrideSelector() will not function the same as TestBed.inject(MockStore).overrideSelector() (as you may not get the same mock store used by your component).

I would like to suggest that the function be renamed to createMockStore() to avoid confusion by making it clear that this will be creating a new mock store rather than getting an existing one (probably with a deprecation period for createMockStore().

Describe any alternatives/workarounds you're currently using

No response

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@timdeschryver
Copy link
Member

I like this suggestion 👍
What do you think @markostanimirovic and @brandonroberts ?

@brandonroberts
Copy link
Member

Seems reasonable to me. As mentioned, we can deprecate the old name. If there is an automatic migration added we can remove it in v16.

@markostanimirovic
Copy link
Member

Sounds good to me too 👍

@zbarbuto do you want to create a PR for this?

You can take a look at these PRs to see how it can be done: #3745 #3753

@zbarbuto
Copy link
Contributor Author

@markostanimirovic yes, PR is now up (co #3789 - hopefully I did everything right)

Happy to do the schematic one as well (looks like that comes after the first part is merged?)

Thanks all for the prompt responses.

@timdeschryver timdeschryver added the community watch Someone from the community is working this issue/PR label Feb 27, 2023
@ValentinBossi
Copy link
Contributor

ValentinBossi commented Jun 6, 2023

i think i see side-effects in my tests with createMockStore.
i have 349 tests and i adapted an effect test file (without Testbed), cause the effect now uses data from a selector (selectTT)(https://v15.ngrx.io/guide/effects#incorporating-state), but now other code in other files throws errors (TypeError: Cannot read properties of undefined (reading 'forEach')...)

before:
effects = new XEffects(actions$, XMockService);
after:
effects = new XEffects(actions$, XMockService, createMockStore({selectors: [{ selector: selectTT, value: { id: 'UUID' } as TT }]}));

is it possible that the selectTT and/or the value is still used later in other tests resp even test files?

EDIT:
wow adding
afterAll(() => { mockSelectors = []; });
(cause im doing actually effects = new XEffects(actions$, XMockService, createMockStore({selectors: mockSelectors}))); )

made the test output even worse. obviously im using this the wrong way. (btw im doing mockSelectors.push(...) in 2 tests and beforeEach mockSelectors = [])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs community watch Someone from the community is working this issue/PR Good First Issue Good issue for first-time contributor Project: Store
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants