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.

Sign up for GitHub

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

Proof of Concept: Enhancer Overhaul #1702

Closed
wants to merge 1 commit into from
Closed

Proof of Concept: Enhancer Overhaul #1702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
229 changes: 64 additions & 165 deletions src/createStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,110 +1,90 @@
import isPlainObject from 'lodash/isPlainObject'
import $$observable from 'symbol-observable'

/**
* These are private action types reserved by Redux.
* For any unknown actions, you must return the current state.
* If the current state is undefined, you must return the initial state.
* Do not reference these action types directly in your code.
*/
export var ActionTypes = {
INIT: '@@redux/INIT'
}

/**
* Creates a Redux store that holds the state tree.
* The only way to change the data in the store is to call `dispatch()` on it.
*
* There should only be a single store in your app. To specify how different
* parts of the state tree respond to actions, you may combine several reducers
* into a single reducer function by using `combineReducers`.
*
* @param {Function} reducer A function that returns the next state tree, given
* the current state tree and the action to handle.
*
* @param {any} [initialState] The initial state. You may optionally specify it
* to hydrate the state from the server in universal apps, or to restore a
* previously serialized user session.
* If you use `combineReducers` to produce the root reducer function, this must be
* an object with the same shape as `combineReducers` keys.
*
* @param {Function} enhancer The store enhancer. You may optionally specify it
* to enhance the store with third-party capabilities such as middleware,
* time travel, persistence, etc. The only store enhancer that ships with Redux
* is `applyMiddleware()`.
*
* @returns {Store} A Redux store that lets you read the state, dispatch actions
* and subscribe to changes.
*/
export default function createStore(reducer, initialState, enhancer) {
if (typeof initialState === 'function' && typeof enhancer === 'undefined') {
enhancer = initialState
initialState = undefined
function createStoreBase(reducer, initialState, onChange) {
var currentState = initialState
var isDispatching = false

function getState() {
return currentState
}

if (typeof enhancer !== 'undefined') {
if (typeof enhancer !== 'function') {
throw new Error('Expected the enhancer to be a function.')
function dispatch(action) {
if (!isPlainObject(action)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I suggest that you move all these opinionated checks into createStore() instead? Seems more appropriate there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way enhancers/middleware would be open to dispatch anything which can cause bugs (e.g. dispatching empty values by mistake). IMO they’re more useful here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah rethinking this, that's fine with me. If someone (me) really wanted to opt-out of those things, they could easily write their own createStoreBase().

But part of me does think it would be nice if there were a "core enhancer" that implemented this stuff, and was automatically applied by createStore. Then createStoreBase could be exported separately (contingent on finding a better name for it), which would allow for alternative createStore-like APIs to be experimented on in userland without having to rewrite much of anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. someone could write a createObservableStore (use any stream/event system here, just providing an example) that is used in place of createStore but is totally compatible with enhancers. Yes, it's possible anyway by composing createStore but this way is cleaner.

throw new Error(
'Actions must be plain objects. ' +
'Use custom middleware for async actions.'
)
}
if (typeof action.type === 'undefined') {
throw new Error(
'Actions may not have an undefined "type" property. ' +
'Have you misspelled a constant?'
)
}
if (isDispatching) {
throw new Error('Reducers may not dispatch actions.')
}

try {
isDispatching = true
currentState = reducer(currentState, action)
} finally {
isDispatching = false
}

return enhancer(createStore)(reducer, initialState)
onChange()
return action
}

return {
dispatch,
getState
}
}

export default function createStore(reducer, initialState, enhancer) {
if (typeof reducer !== 'function') {
throw new Error('Expected the reducer to be a function.')
}
if (typeof initialState === 'function' && typeof enhancer === 'undefined') {
enhancer = initialState
initialState = undefined
}
if (typeof enhancer !== 'undefined' && typeof enhancer !== 'function') {
throw new Error('Expected the enhancer to be a function.')
}

var currentReducer = reducer
var currentState = initialState
enhancer = enhancer || (x => x)
var createFinalStoreBase = enhancer(createStoreBase)

var storeBase
var currentListeners = []
var nextListeners = currentListeners
var isDispatching = false

function onChange() {
var listeners = currentListeners = nextListeners
for (var i = 0; i < listeners.length; i++) {
listeners[i]()
}
}

function ensureCanMutateNextListeners() {
if (nextListeners === currentListeners) {
nextListeners = currentListeners.slice()
}
}

/**
* Reads the state tree managed by the store.
*
* @returns {any} The current state tree of your application.
*/
function getState() {
return currentState
}

/**
* Adds a change listener. It will be called any time an action is dispatched,
* and some part of the state tree may potentially have changed. You may then
* call `getState()` to read the current state tree inside the callback.
*
* You may call `dispatch()` from a change listener, with the following
* caveats:
*
* 1. The subscriptions are snapshotted just before every `dispatch()` call.
* If you subscribe or unsubscribe while the listeners are being invoked, this
* will not have any effect on the `dispatch()` that is currently in progress.
* However, the next `dispatch()` call, whether nested or not, will use a more
* recent snapshot of the subscription list.
*
* 2. The listener should not expect to see all state changes, as the state
* might have been updated multiple times during a nested `dispatch()` before
* the listener is called. It is, however, guaranteed that all subscribers
* registered before the `dispatch()` started will be called with the latest
* state by the time it exits.
*
* @param {Function} listener A callback to be invoked on every dispatch.
* @returns {Function} A function to remove this change listener.
*/
function subscribe(listener) {
if (typeof listener !== 'function') {
throw new Error('Expected listener to be a function.')
}

var isSubscribed = true

ensureCanMutateNextListeners()
nextListeners.push(listener)

Expand All @@ -114,121 +94,43 @@ export default function createStore(reducer, initialState, enhancer) {
}

isSubscribed = false

ensureCanMutateNextListeners()
var index = nextListeners.indexOf(listener)
nextListeners.splice(index, 1)
}
}

/**
* Dispatches an action. It is the only way to trigger a state change.
*
* The `reducer` function, used to create the store, will be called with the
* current state tree and the given `action`. Its return value will
* be considered the **next** state of the tree, and the change listeners
* will be notified.
*
* The base implementation only supports plain object actions. If you want to
* dispatch a Promise, an Observable, a thunk, or something else, you need to
* wrap your store creating function into the corresponding middleware. For
* example, see the documentation for the `redux-thunk` package. Even the
* middleware will eventually dispatch plain object actions using this method.
*
* @param {Object} action A plain object representing “what changed”. It is
* a good idea to keep actions serializable so you can record and replay user
* sessions, or use the time travelling `redux-devtools`. An action must have
* a `type` property which may not be `undefined`. It is a good idea to use
* string constants for action types.
*
* @returns {Object} For convenience, the same action object you dispatched.
*
* Note that, if you use a custom middleware, it may wrap `dispatch()` to
* return something else (for example, a Promise you can await).
*/
function dispatch(action) {
if (!isPlainObject(action)) {
throw new Error(
'Actions must be plain objects. ' +
'Use custom middleware for async actions.'
)
}

if (typeof action.type === 'undefined') {
throw new Error(
'Actions may not have an undefined "type" property. ' +
'Have you misspelled a constant?'
)
}

if (isDispatching) {
throw new Error('Reducers may not dispatch actions.')
}

try {
isDispatching = true
currentState = currentReducer(currentState, action)
} finally {
isDispatching = false
}

var listeners = currentListeners = nextListeners
for (var i = 0; i < listeners.length; i++) {
listeners[i]()
}
return storeBase.dispatch(action)
}

return action
function getState() {
return storeBase.getState()
}

/**
* Replaces the reducer currently used by the store to calculate the state.
*
* You might need this if your app implements code splitting and you want to
* load some of the reducers dynamically. You might also need this if you
* implement a hot reloading mechanism for Redux.
*
* @param {Function} nextReducer The reducer for the store to use instead.
* @returns {void}
*/
function replaceReducer(nextReducer) {
if (typeof nextReducer !== 'function') {
throw new Error('Expected the nextReducer to be a function.')
}

currentReducer = nextReducer
var nextInitialState = storeBase ? getState() : initialState
Copy link
Contributor

@jridgewell jridgewell Sep 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, initialState will never be garbage collected. You'd either need to move the initial storeBase creation outside of replaceReducer, or null it out after this.

storeBase = createFinalStoreBase(nextReducer, nextInitialState, onChange)
Copy link
Collaborator

@acdlite acdlite May 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, now we can throw here if the enhancer returns the wrong thing! Sweet

dispatch({ type: ActionTypes.INIT })
}

/**
* Interoperability point for observable/reactive libraries.
* @returns {observable} A minimal observable of state changes.
* For more information, see the observable proposal:
* https://github.com/zenparsing/es-observable
*/
function observable() {
var outerSubscribe = subscribe
return {
/**
* The minimal observable subscription method.
* @param {Object} observer Any object that can be used as an observer.
* The observer object should have a `next` method.
* @returns {subscription} An object with an `unsubscribe` method that can
* be used to unsubscribe the observable from the store, and prevent further
* emission of values from the observable.
*/
subscribe(observer) {
if (typeof observer !== 'object') {
throw new TypeError('Expected the observer to be an object.')
}

function observeState() {
if (observer.next) {
observer.next(getState())
}
}

observeState()
var unsubscribe = outerSubscribe(observeState)
var unsubscribe = subscribe(observeState)
return { unsubscribe }
},

Expand All @@ -238,15 +140,12 @@ export default function createStore(reducer, initialState, enhancer) {
}
}

// When a store is created, an "INIT" action is dispatched so that every
// reducer returns their initial state. This effectively populates
// the initial state tree.
dispatch({ type: ActionTypes.INIT })
replaceReducer(reducer)

return {
dispatch,
Copy link
Collaborator

@acdlite acdlite May 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the "real" store extend the store base here via spread, so that enhancers can add their own properties besides dispatch and getState?

return {
  ...storeBase,
  dispatch,
  getState,
  subscribe,
  // ... and so on
}

E.g. this would allow the observable interface to be safely be implemented as an enhancer. Not that we'd necessarily want to do that — just an example of how alternative event systems* are possible with this new enhancer architecture.

*not that they weren't possible before, but now they're safer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, I don't see the benefit of "sealing" the enhancers, and it seems like you lose some extensibility as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I tried to solve is that anything other than { getState, dispatch } is hard to compose, and enhancers have no knowledge of the past enhancers. If any enhancer could add a method like subscribe(), it would be a pain to later batch subscriptions in any later enhancer (this is pretty much why https://github.com/tappleby/redux-batched-subscribe is currently using a less performant subscription mechanism which was copy pasted from earlier versions of Redux). If any enhancer could add [$$observable], then any further enhancer changing getState would cause it to report incorrect values because the inner enhancer uses the inner getState.

Sealing off limits the domain of enhancers to:

  • Hijacking reducer
  • Hijacking initialState
  • Hijacking dispatch
  • Hijacking getState
  • Hijacking onChange

All these things are guaranteed to be orthogonal: it is always possible to write an enhancer that changes some of them and it is guaranteed that this won’t trip up enhancers before or after it.

However as soon as we let enhancers define non-orthogonal methods (such as [$$observable] which depends on subscribe and getState), this introduces subtle failures. If a default enhancer included subscribe, a thing that wants to delay subscribe would have to reimplement its logic.

I feel good that all existing use cases seem covered by (reducer, initialState, onChange) => { dispatch, getState }. I would be wary of making the system more expressive because this caused issues in the past. Most enhancers so far had to work around this freedom rather than enjoy it. Finally, for what it’s worth, it is always possible to really wrap createStore if you really need it. I’m just not convinced this is any close to being a common case for the enhancers I have seen.

subscribe,
getState,
subscribe,
replaceReducer,
[$$observable]: observable
}
Expand Down
18 changes: 0 additions & 18 deletions test/applyMiddleware.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,22 +94,4 @@ describe('applyMiddleware', () => {
done()
})
})

it('keeps unwrapped dispatch available while middleware is initializing', () => {
// This is documenting the existing behavior in Redux 3.x.
// We plan to forbid this in Redux 4.x.

function earlyDispatch({ dispatch }) {
dispatch(addTodo('Hello'))
return () => action => action
}

const store = createStore(reducers.todos, applyMiddleware(earlyDispatch))
expect(store.getState()).toEqual([
{
id: 1,
text: 'Hello'
}
])
})
})
22 changes: 16 additions & 6 deletions test/createStore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,22 +495,27 @@ describe('createStore', () => {
})

it('accepts enhancer as the third argument', () => {
const spyActions = []
const emptyArray = []
const spyEnhancer = vanillaCreateStore => (...args) => {
expect(args[0]).toBe(reducers.todos)
expect(args[1]).toBe(emptyArray)
expect(args.length).toBe(2)
expect(args[2]).toBeA('function')
expect(args.length).toBe(3)
const vanillaStore = vanillaCreateStore(...args)
return {
...vanillaStore,
dispatch: expect.createSpy(vanillaStore.dispatch).andCallThrough()
dispatch(action) {
spyActions.push(action)
return vanillaStore.dispatch(action)
}
}
}

const store = createStore(reducers.todos, emptyArray, spyEnhancer)
const action = addTodo('Hello')
store.dispatch(action)
expect(store.dispatch).toHaveBeenCalledWith(action)
expect(spyActions).toContain(action)
expect(store.getState()).toEqual([
{
id: 1,
Expand All @@ -520,21 +525,26 @@ describe('createStore', () => {
})

it('accepts enhancer as the second argument if initial state is missing', () => {
const spyActions = []
const spyEnhancer = vanillaCreateStore => (...args) => {
expect(args[0]).toBe(reducers.todos)
expect(args[1]).toBe(undefined)
expect(args.length).toBe(2)
expect(args[2]).toBeA('function')
expect(args.length).toBe(3)
const vanillaStore = vanillaCreateStore(...args)
return {
...vanillaStore,
dispatch: expect.createSpy(vanillaStore.dispatch).andCallThrough()
dispatch(action) {
spyActions.push(action)
return vanillaStore.dispatch(action)
}
}
}

const store = createStore(reducers.todos, spyEnhancer)
const action = addTodo('Hello')
store.dispatch(action)
expect(store.dispatch).toHaveBeenCalledWith(action)
expect(spyActions).toContain(action)
expect(store.getState()).toEqual([
{
id: 1,
Expand Down