Change all containers in base components to use context for selectors#740
Change all containers in base components to use context for selectors#740shorja wants to merge 1 commit intoGriddleGriddle:masterfrom
Conversation
|
Love it! I think we can pretty confident this isn't a breaking change, simply because we're not changing the exported selectors that plugins have had to reference until now. This does make selectors an even more explicit part of the public API, though, so we'll need to be more careful about 1) deciding which to export, and 2) how they are designed (esp. with regard to exposing the immutable internals). While we're at it, any thoughts on avoiding this pattern? |
|
Very cool 👍 @dahlbyk regarding the extra |
|
On further consideration, that particular pattern isn't the problem (though it will be nice to get rid of all the copy/paste). I'm glad to contribute that cleanup if @shorja doesn't wish to add it to this PR. The pattern I meant to ask about manifests here: |
|
Yeah, cascading dependencies are a bit of an issue at the moment. I believe @ryanlanciaux (who's currently in transit) can speak a little more about this, but I think he's been working on something that should handle some of the selector composition issues. |
|
@dahlbyk @joellanciaux @ryanlanciaux I'm also working on a solution to the problem of duplicated selectors in plugins, I'll post it in another PR since its not specifically related and might be a bit more of a dangerous change, should have that up sometime today. |
dahlbyk
left a comment
dahlbyk
left a comment
There was a problem hiding this comment.
Added some feedback on the code; feel free to squash those changes.
Can you also check if any of the plugin components could use context for selectors?
| // cellPropertiesSelector, | ||
| // classNamesForComponentSelector, | ||
| // stylesForComponentSelector | ||
| //} from '../selectors/dataSelectors'; |
There was a problem hiding this comment.
Please remove the commented lines.
| )(props => ( | ||
| <OriginalComponent | ||
| {...props} | ||
| {...props} |
There was a problem hiding this comment.
Please move this onto a single line, like the other components:
)(props => <OriginalComponent {...props} />);It occurs to me that this is essentially a HOC identity function. @ryanlanciaux @joellanciaux is there a reason you haven't returned the result of compose() directly?
-const ComposedRowContainer = OriginalComponent => compose(
+const ComposedRowContainer = compose(
...
-)(props => <OriginalComponent {...props} />);
+);|
|
||
| const enhancedSettingsToggle = OriginalComponent => compose( | ||
| getContext({ | ||
| selectors: PropTypes.object |
There was a problem hiding this comment.
Here, and elsewhere, please ensure you have a trailing comma for consistency.
| rowIds={visibleRowIds} | ||
| Row={Row} | ||
| style={style} | ||
| className={className} |
There was a problem hiding this comment.
This component can pass-thru props directly, like the other components:
)(props => <OriginalComponent {...props} />);There was a problem hiding this comment.
Clarification: please rename visibleRowIds to rowIds so props can pass-thru directly.
| ), | ||
| mapProps(props => { | ||
| const { components, ...otherProps } = props; | ||
| const { components, selectors, ...otherProps } = props; |
There was a problem hiding this comment.
I noticed this is the only place you're pulling selectors out of otherProps. I think it's better this way, since selectors isn't meant to be exposed to the wrapped component - can you add repeat this change elsewhere?
3 participants
Griddle major version
1.9.0
Changes proposed in this pull request
Convert all references to dataSelectors file in components to the context.selectors
Why these changes are made
This allows plugins to override selectors in the base components without having to wholesale copy them. Would this potentially be a dangerous change from a consumers of Griddle perspective?
Are there tests?
No