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

compatibility with reselect? #118

Closed
karptonite opened this issue Jul 19, 2017 · 8 comments
Closed

compatibility with reselect? #118

karptonite opened this issue Jul 19, 2017 · 8 comments

Comments

@karptonite
Copy link
Contributor

I see that ngrx 4 now has its own implementation of createSelector(). However, will it still be compatible with reselect, should we choose to use it for some of the more advanced features that reselect offers?

I'm a bit concerned in particular about this bit of code here:

} else if (typeof pathOrMapFn === 'function' && isSelector(pathOrMapFn)) {
mapped$ = map.call(this, pathOrMapFn);
} else if (typeof pathOrMapFn === 'function') {
mapped$ = map.call(this, createSelector(s => s, pathOrMapFn));

where it looks as if the code checks isSelector() (for which I think a reselect selector would return false) then wraps the selector in an ngrx selector if it is not a selector, possibly leading to the result being "double memoized".

@alexjlockwood
Copy link

+1, I am also confused about whether I should be using @ngrx/stores custom createSelector function or if it is still OK to use reselect. I think I am facing the same issue @karptonite pointed out as well.

@karptonite
Copy link
Contributor Author

I may also be confused about what sort of memoization is going on in createSelector vs what sort of memoization is going on in Store.select().

@MikeRyanDev
Copy link
Member

Ah, good point about that line of code. Our intention is certainly to maintain compatibility with Reselect because they offer more advanced features. I will happily accept a PR that simplifies that code path.

@karptonite
Copy link
Contributor Author

@MikeRyanDev One solution is to just remove the part of store where it wraps a selector in createSelector if it is not already a selector. That puts the onus on the user to add createSelector, but it also allows the user to omit memoization if they have a selector where memoization causes a performance hit.

@karptonite
Copy link
Contributor Author

In general, I'm not sure that the benefits outweigh the added technical debt for ngrx implementing its own version of reselect. reselect is a pretty small library. I know you need it if you want to add createFeatureSelector without adding a dependency on reselect, but a feature selector is such a simple selector that it seems overkill.

@alexjlockwood
Copy link

+1, totally agree. Setting up reselect is super easy and familiar, especially if you come from a redux background. If you really wanted to integrate the ability to create selectors into @ngrx/store, I'm not sure why you wouldn't just depend on reselect directly. But it also doesn't seem like something you'd want to force on developers either.

@MikeRyanDev
Copy link
Member

@karptonite Sounds like a good solution to me.

Also, our own memoized selector implementation is very little code. It was put in place so that we didn't need any dependencies and to put in groundwork for future improvements. It seems very likely that future ngrx libraries will be providing their own selectors.

@karptonite
Copy link
Contributor Author

I'll get out a pull request.

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

No branches or pull requests

3 participants