Composable selectors #743
Composable selectors #743
Conversation
|
I will add some examples to storybook tomorrow to demonstrate how this works too |
|
Here is a diagram to help explain what I'm trying to do, I can do some more if necessary later
|
|
This is really great -- Going to spend some time going through this a bit but really promising and cool functionality! Thank you for adding this! |
| @@ -98,7 +99,206 @@ class Griddle extends Component { | |||
|
|
|||
| this.events = Object.assign({}, events, ...plugins.map(p => p.events)); | |||
|
|
|||
| this.selectors = plugins.reduce((combined, plugin) => ({ ...combined, ...plugin.selectors }), {...selectors}); | |||
There was a problem hiding this comment.
Before we dig into reviewing this code, can you pull your whole new block into ./utils/selectorUtils.js to minimize the churn here? Something like:
this.selectors = flattenSelectors(selectors, plugins);
That should make it a lot easier to test your selector composition logic in isolation.
(Please squash this change, to avoid merge conflicts in this file.)
src/selectors/composedSelectors.js
Outdated
| import MAX_SAFE_INTEGER from 'max-safe-integer' | ||
|
|
||
| export const hasPreviousSelector = { | ||
| creator: ({currentPageSelector}) => { |
There was a problem hiding this comment.
This may be too clever, but I wonder if this would be easier to write/read by attaching dependencies directly to the creator function:
export const hasPreviousSelector = ({ currentPageSelector }) =>
createSelector(
currentPageSelector,
currentPage => currentPage > 1);
hasPreviousSelector.dependencies = ["currentPageSelector"];
…a composable selector generator function to be used to create composable selectors.
* master:
Use Loading component when data={null|undefined}
Handle data={null|undefined}
Wire up Redux DevTools
|
To clarify, the example story is a minimalist implementation of Seeing your |
|
Ok so I've been looking at your refactor, I really like the idea of a drop in replacement for createSelector that is able to adapt to whichever way it is called to replicate old behaviour. I have been working on a refactor to your refactor too. The way it was implemented with the global selectors, all of the import style references to selectors are recreated every time they are called which prevents the use of the auto memoization feature that reselect provides. I haven't quite finished my refactor but it should prevent this from being a problem and also the globalSelectors shouldn't be necessary. However, I am concerned with the implications of changing the dataSelectors over to use the new dependency style. I'm worried that this could introduce some confusing behaviour for existing users especially since 'hard' imported selectors could now start being overridden without them knowing why. This was my goal with the original design of having a separate composedSelectors file, though this will still provide the overriding selector behaviour on the context which may also be a breaking change for existing users. So, I'm going to propose making a change to the way the core components, selectors, reducers, etc... are used. I'm going to try moving this 'core' functionality over to a plugin. Griddle will always load one 'baseline' plugin, and by default it will always load the 'core' plugin. Then we can implement a new plugin, that has all of the same features of the 'core' plugin but will use all of the new composability features. Existing plugin users should still be able to leverage new composability features if they choose to use them but they won't be automagically opted in. This will also allow developers to totally override the 'baseline' with their own so that they don't have a cumbersome overriding process. This gets pretty close to Griddle being just a component building framework but is still backwards compatible. I'll put up a PR as soon as I can get it done. |
@ryanlanciaux and @joellanciaux, or other users, may disagree, but I'm not too concerned about people having taken hard dependencies on the selectors that have been exported thus far, because there hasn't been any way to inject dependencies until now. In other words, existing behavior can't change unless a new plugin/customization is added that uses this DI mechanism. I do like the idea of thinking of the Griddle core behavior as a plugin; I am actually working on a branch that essentially takes that same view in refactoring how plugins are composed (#733). I will try to share that sooner rather than later, for your consideration. |
|
@dahlbyk My concern is specifically with changing the dataSelectors to using the quoted versions and their interaction with the core code. For example if a user had defined a custom selector in their plugin that had the same name as a dataSelectors one, the current behaviour would be that it would be overridden in the context object but of course all of the hard references to it would stay the same whereas if the dataSelectors are created with the new logic using quoted dependencies, the 'hard' references will be overridden and all of the core code will now use that new selector. I'm more concerned about all of the code that dev's didn't write magically behaving differently. Though if we look at PR 740, adding the selectors to the context will also potentially cause unexpected behaviour for devs not familiar with any new composability features since that would ALSO make all the core code magically use any overridden selectors. Sorry, I'm not trying to be contrarian, I'm just probably overly cautious about messing things up. |
|
@dahlbyk Oh and yeah I would love to see your approach to a plugin style refactor for the core code. |
|
@shorja check out #757. The work is a few months old, so doesn't account for new functionality, but the key piece is that the initializer essentially accepts the default reducers, components, selectors, etc. in the shape of a plugin: https://github.com/dahlbyk/Griddle/blob/65ff192e3d8cef77a7975d372a3752fa3aa7b650/src/index.js#L69-L86. |
|
Can we expect this to be merged anytime soon ? Being able to override the Local Plugin's filtered data selector, or any selector, without modifying any of its dependencies would be incredibly useful. |
|
I too would like to see this functionality added! |
Griddle major version
1.9.0
Changes proposed in this pull request
This is a potential upgrade to the existing way selectors are created.
Why these changes are made
Selectors are currently limited in how they can be overridden, this change would make it possible to override any number of selectors without having to change the dependency selectors. In combination with this PR which changes all the built-in components to use the selectors on the context, this would allow for much more powerful overriding of selectors.
If this is a desirable feature, we need to have a discussion about how to make sure this isn't a breaking change, maybe this could be enabled by a flag on the Griddle component. Without a plugin using the new composableSelectors file, I believe this SHOULD be backwards compatible, would need to do some testing.
Are there tests?
Not yet, needs comments first