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

feat(store): use provideMockStore outside of the TestBed #2759

Merged
merged 2 commits into from
Nov 15, 2020

Conversation

markostanimirovic
Copy link
Member

@markostanimirovic markostanimirovic commented Oct 22, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

provideMockStore can only be used with TestBed.configureTestingModule.

Closes #2745

What is the new behavior?

provideMockStore can be used with TestBed.configureTestingModule and Injector.create.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Oct 22, 2020

Preview docs changes for 93d567a at https://previews.ngrx.io/pr2759-93d567a0/

@markostanimirovic markostanimirovic changed the title feat(store): create Store/MockStore outside of the TestBed feat(store): use provideMockStore outside of the TestBed Oct 23, 2020
@markostanimirovic markostanimirovic force-pushed the provide-mock-store branch 2 times, most recently from ce217ec to d63fc80 Compare October 26, 2020 18:40
@markostanimirovic markostanimirovic marked this pull request as ready for review October 26, 2020 18:59
@alex-okrushko
Copy link
Member

🤔
I like the idea @markostanimirovic 👍
Let me think about it a bit to see if it can be further iterated.

Copy link
Member

@alex-okrushko alex-okrushko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like where this is going!

* });
* });
* ```
*/
export function provideMockStore<T = any>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the following?

This is close to what you had, but I want to make sure that useExisting is used, and it's a bit more compacted.

Suggested change
export function provideMockStore<T = any>(
export function provideMockStore<T = any>(
config: MockStoreConfig<T> = {}
): Provider[] {
setNgrxMockEnvironment(true);
return [
{ provide: ActionsSubject, useFactory: () => new ActionsSubject(), deps: []},
{ provide: MockState, useFactory: () => new MockState<T>(), deps: []},
{ provide: MockReducerManager, useFactory: () => new MockReducerManager(), deps: []},
{ provide: INITIAL_STATE, useValue: config.initialState || {} },
{ provide: MOCK_SELECTORS, useValue: config.selectors },
{ provide: StateObservable, useExisting: MockState },
{ provide: ReducerManager, useExisting: MockReducerManager },
{
provide: MockStore,
useFactory: mockStoreFactory,
deps: [
MockState,
ActionsSubject,
ReducerManager,
INITIAL_STATE,
MOCK_SELECTORS,
],
},
{ provide: Store, useExisting: MockStore },
];
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-okrushko
Done. I assumed that StateObservable needs useExisting: MockState, but I added useFactory: () => new MockState() in order to keep the same behavior as previous { provide: StateObservable, useClass: MockState }.

About return type, I kept (ValueProvider | ExistingProvider | FactoryProvider)[], because if Provider[] is used, following code will not compile:

injector = Injector.create({ providers: provideMockStore(...) });

because Provider is not compatible with StaticProvider and (ValueProvider | ExistingProvider | FactoryProvider) is compatible with both Provider and StaticProvider.

mockSelectors
);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of going through the Injector every time, maybe let's create the getMockStore or createMockStore function:

Suggested change
export function getMockStore<T>(
config: MockStoreConfig<T> = {}
): MockStore<T> {
return Injector.create({ providers: [ provideMockStore(config) ]}).get(MockStore);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

modules/store/testing/spec/mock_store.spec.ts Outdated Show resolved Hide resolved
MockStore,
{
provide: ActionsSubject,
useFactory: () => new ActionsSubject(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use useClass here?

Suggested change
useFactory: () => new ActionsSubject(),
useClass: ActionsSubject,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandonroberts Because class providers are different for TestBed.configureTestingModule and Injector.create. TestBed.configureTestingModule expects ClassProvider. On the other hand, Injector.create expects StaticClassProvider. However, they both accept FactoryProvider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, what about just { provide: ActionsSubject, deps: [] }? I know it's compatible with the Injector, and should be compatible with the TestBed also

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason the ActionsSubject is undefined in this case.

useFactory: () => new ActionsSubject(),
deps: [],
},
{ provide: MockState, useFactory: () => new MockState<T>(), deps: [] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with useClass

{ provide: MockState, useFactory: () => new MockState<T>(), deps: [] },
{
provide: MockReducerManager,
useFactory: () => new MockReducerManager(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with useClass

@brandonroberts
Copy link
Member

This would be a separate effort, but what about creating a factory function for creating a Store also? I did this as a POC for using NgRx Store in React a while back.

https:/brandonroberts/react-ngrx-poc/blob/master/src/store.tsx

cc: @alex-okrushko

Copy link
Member

@alex-okrushko alex-okrushko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @markostanimirovic 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store: add ability to create the Store/MockStore outside of the TestBed
4 participants