Refactor dispatch and getStore naming #203
Conversation
anacierdem
left a comment
anacierdem
left a comment
There was a problem hiding this comment.
Looks OK to me, just a few questions.
| .spyOn(Promise, 'reject') | ||
| .mockImplementation(() => {}); | ||
| it('should throw an error if contained store is used without container', () => { | ||
| const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); |
There was a problem hiding this comment.
Looks like this is not used anymore.
There was a problem hiding this comment.
This is needed to silence React using console.error when we throw an error during render 😒
https://stackoverflow.com/a/66329699
| getStore = ( | ||
| Store, | ||
| scopeId = this.defaultScope, | ||
| config = { props: () => ({}), contained: () => false } |
There was a problem hiding this comment.
Just a heads up, if only one of them is provided (props or container) the other will not have a default as one might expect.
There was a problem hiding this comment.
Yeah, might check again how to improve it later, as with #201 we will likely add another key
| `Store ${Store.key} should be contained by a container but it is used globally. ` + | ||
| `While it might still work, it will likely cause unexpected behaviours.` | ||
| ) | ||
| if (Store.containedBy && !config.contained(Store)) { |
There was a problem hiding this comment.
It is not apparent to me why we pass in a predicate while we can already check the result at the call site?
There was a problem hiding this comment.
True, right now it is superfluous. I ported it from #201 where Store can be a different one but maybe we can evaluate at call site even there. Will check and eventually simplify in that PR.
A couple of minor improvements to help future developments (eg #201 ):
bindActionsarguments anddispatchcontext.getStoretoretrieveStoreto clarify API difference fromregistry.getStoreThere should be no public API changes as all this is internal stuff or unreleased