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

Make createStoreBase less opinionated by moving checks into coreEnhancer #1706

Closed
wants to merge 1 commit into from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented May 9, 2016

Following up on my comment from #1702, this PR makes createStoreBase less opinionated by moving certain invariants (actions must be plain objects; actions must have type field) into a coreEnhancer that is applied automatically by createStore. This allows for a future in which createStoreBase (or whatever we end up naming it) can be exported separately, providing a cleaner integration point for userland experiments and alternative createStore APIs that may want to opt-out of those assumptions that, while they may make sense for the majority of Redux consumers, aren't essential to the nature of Redux.

With this separation, createStore is now almost exclusively concerned with subscriptions. I like this because it should make all parties happier: tinkerers can opt-out of "conveniences" that they don't like (such as the INIT action) without having to rewrite any subscription logic, which in turn makes the barrier to adding more conveniences to createStore lower. This is similar in spirit to how we currently treat combineReducers: if you don't like how it works, you don't have to use it.

@gaearon
Copy link
Contributor

gaearon commented May 18, 2016

The downsides I see to this is that

  • It makes each dispatch yet another level deeper makes debugging a little more painful.
  • I’m not sure there’s really a “market” for createStoreBase users, but adding another public API is a big step. If this started as a third-party library and gained traction, I can imagine adding more API surface but I’m not sure I see the benefits yet.
  • If we’re not adding public API surface right now, I don’t see enough value in adding this indirection.

Redux is a mildly opinionated piece of software, and I think we need to let the ecosystem guide our direction and help us draw the boundaries. In #1702, my goal was to help the existing popular ecosystem packages (redux-devtools, redux-loop, redux-batched-subscribe, redux-saga, new Rx interop) employ less hacks. I would be comfortable making the change here if I saw several popular packages that would benefit from this.

@acdlite
Copy link
Collaborator Author

acdlite commented May 19, 2016

I would be comfortable making the change here if I saw several popular packages that would benefit from this.

In my opinion, this is a chicken-and-egg problem: the libraries that would most benefit from this don't exist yet because until #1702 is released, they're a bit of a pain in the ass to create.

I’m not sure there’s really a “market” for createStoreBase users, but adding another public API is a big step. If this started as a third-party library and gained traction, I can imagine adding more API surface but I’m not sure I see the benefits yet.

I should have been clearer. It's not so much createStoreBase that I'd be interested in exporting, but rather coreEnhancer.

Potential example: we want to create a local, nested-Redux type architecture, like the Elm Architecture. Reducers and and actions are colocated with corresponding React components. Each component becomes it's own little mini-Redux app, though in reality, they all nest together to form one unified state tree.

Say we want to be able apply middleware and enhancers at any particular level of the component tree (as opposed to normal Redux, where enhancers are applied at a global level). This involves bypassing createStore entirely and creating a "simulated" store that conforms to the same interface. #1702 makes our enhancer-compatibility goal much simpler, especially since we don't have to worry about the INIT action, which doesn't make sense for our library.

By bypassing createStore, we're also missing out on the event system. Also fine, since in our case we'll use React's update cycle to propagate changes instead.

So far so good. But in order to be good Redux community citizens, we want to enforce certain Redux-invariants—like "actions must be plain-objects" and "action.type must be defined". Because those are currently embedded directly in createStore, we have to rewrite them for our new library.

Another type of library I had in mind were higher-level alternatives to createStore, for people who want even more opinions and framework-like features. Currently, they only work by wrapping Redux's createStore, which may not be ideal if, say, they don't need Redux's event system or its Observable interop, but they don't want to reimplement the rest of the core logic.

Yet another type of library might want to swap out Redux's event system for a different system like xstream or something. Currently this is only possible by

  1. building on top of the existing event system, or
  2. again, rewriting createStore from scratch.

Option 1 might not be desired because, as you said in #1729, there are many ways to implement a change emitter. Option 2 becomes much more palatable if there's a coreEnhancer that allows you to avoid reimplementing those conveniences.

Final note: I confess that with a library as small as Redux, this isn't a huge deal. This may be much ado about nothing.

@gaearon
Copy link
Contributor

gaearon commented May 19, 2016

Final note: I confess that with a library as small as Redux, this isn't a huge deal. This may be much ado about nothing.

“Much ado about nothing” should’ve been our GitHub tagline.

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.

3 participants