Breaking API changes for 1.0 #195

Merged
merged 129 commits into from Aug 14, 2015

Conversation

@gaearon
Collaborator

gaearon commented Jun 30, 2015

Naming

API changes

  • composeStores is now composeReducers.
  • createDispatcher is gone.
  • createRedux is now createStore.
  • <Provider> now accepts store prop instead of redux.
  • The new createStore signature is createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array).
  • If the first argument to createStore is an object, composeReducers is automatically applied to it.
  • The “smart” middleware signature changed. It now accepts an object instead of a single getState function. The dispatch function lets you “recurse” the middleware chain and is useful for async: #113 (comment).

Correctness changes

  • The dispatch provided by the default thunk middleware now walks the whole middleware chain.
  • It is enforced now that raw Actions at the end of the middleware chain have to be plain objects.
  • Nested dispatches are now handled gracefully. (#110, #119)

Internal changes

  • The object in React context is renamed from redux to store.
  • Some tests are rewritten for clarity, focus and edge cases.
  • Redux in examples is now aliased to the source code for easier work on master.

This was referenced Jun 30, 2015

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 30, 2015

Collaborator

This PR supersedes #166, #119, and #120, and brings us much closer to 1.0 (#164).

The missing pieces for 1.0 are more strict composeReducers (#193), new docs (#140), and a more convenient action creator binding API (#86), if we are certain that we need it.

Collaborator

gaearon commented Jun 30, 2015

This PR supersedes #166, #119, and #120, and brings us much closer to 1.0 (#164).

The missing pieces for 1.0 are more strict composeReducers (#193), new docs (#140), and a more convenient action creator binding API (#86), if we are certain that we need it.

Breaking API changes for 1.0
Naming:

* “Stateless Stores” are now called reducers. (#137 (comment))
* The “Redux instance” is now called “The Store”. (#137 (comment))
* The dispatcher is removed completely. (#166 (comment))

API changes:

* <s>`composeStores`</s> is now `composeReducers`.
* <s>`createDispatcher`</s> is gone.
* <s>`createRedux`</s> is now `createStore`.
* `<Provider>` now accepts `store` prop instead of <s>`redux`</s>.
* The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`.
* If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it.
* The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: #113 (comment).

Correctness changes:

* The `dispatch` provided by the default thunk middleware now walks the whole middleware chain.
* It is enforced now that raw Actions at the end of the middleware chain have to be plain objects.
* Nested dispatches are now handled gracefully. (#110)

Internal changes:

* The object in React context is renamed from <s>`redux`</s> to `store`.
* Some tests are rewritten for clarity, focus and edge cases.
* Redux in examples is now aliased to the source code for easier work on master.
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 30, 2015

Collaborator

Released on NPM as 1.0.0-alpha.

Collaborator

gaearon commented Jun 30, 2015

Released on NPM as 1.0.0-alpha.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jun 30, 2015

Contributor

🎉 :)

Contributor

dariocravero commented Jun 30, 2015

🎉 :)

@@ -1,5 +1,5 @@
import React from 'react';
-import { bindActionCreators } from 'redux';
+import { bindActionCreators } from 'redux/index';

This comment has been minimized.

@gaearon

gaearon Jun 30, 2015

Collaborator

Whoops, this slipped in by mistake when copy-pasting.

@gaearon

gaearon Jun 30, 2015

Collaborator

Whoops, this slipped in by mistake when copy-pasting.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jun 30, 2015

Collaborator

Yay! Looks good!

I've been wondering if composeReducers() is really the best name for that function. The name is very generic, whereas its implementation is very specific.

Consider another common way of composing reducers, which is to apply each one of them in sequence:

(...reducers) => (prevState, action) =>
  reducers.reduce(
    (state, r) => r(state, action),
    prevState
  );

This could also be named composeReducers(). (I'd actually argue this is better fit for the name.)

Maybe we should think of a more specific name? Don't have any suggestions right now, but I wonder what everyone else's thoughts are.

Collaborator

acdlite commented Jun 30, 2015

Yay! Looks good!

I've been wondering if composeReducers() is really the best name for that function. The name is very generic, whereas its implementation is very specific.

Consider another common way of composing reducers, which is to apply each one of them in sequence:

(...reducers) => (prevState, action) =>
  reducers.reduce(
    (state, r) => r(state, action),
    prevState
  );

This could also be named composeReducers(). (I'd actually argue this is better fit for the name.)

Maybe we should think of a more specific name? Don't have any suggestions right now, but I wonder what everyone else's thoughts are.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 30, 2015

Collaborator

@acdlite Any precedents in other libraries' APIs?

Collaborator

gaearon commented Jun 30, 2015

@acdlite Any precedents in other libraries' APIs?

src/middleware/thunk.js
-
- return recurse;
- };
+export default function thunkMiddleware({ dispatch, getState }) {

This comment has been minimized.

@dbrans

dbrans Jul 1, 2015

Contributor

Where does the name "thunk" come from?

@dbrans

dbrans Jul 1, 2015

Contributor

Where does the name "thunk" come from?

This comment has been minimized.

This comment has been minimized.

src/utils/composeReducers.js
+export default function composeReducers(reducers) {
+ const finalReducers = pick(reducers, (val) => typeof val === 'function');
+
+ return function Composition(atom = {}, action) {

This comment has been minimized.

@dbrans

dbrans Jul 1, 2015

Contributor

A few questions:

  1. Why are you calling this "atom" and not "state" or "slice"?
  2. Why does atom have a default value?
  3. why is Composition capitalized?
@dbrans

dbrans Jul 1, 2015

Contributor

A few questions:

  1. Why are you calling this "atom" and not "state" or "slice"?
  2. Why does atom have a default value?
  3. why is Composition capitalized?

This comment has been minimized.

@gaearon

gaearon Jul 1, 2015

Collaborator

@dbrans

  1. “Atom” is leftover from earlier versions of API. Need to be changed to state.
  2. We want the default values for reducers to be undefined. For state[key] to be undefined initially, state needs to be {}. If it's undefined itself, state[key] will just crash.
  3. You're right, we should lower case that.
@gaearon

gaearon Jul 1, 2015

Collaborator

@dbrans

  1. “Atom” is leftover from earlier versions of API. Need to be changed to state.
  2. We want the default values for reducers to be undefined. For state[key] to be undefined initially, state needs to be {}. If it's undefined itself, state[key] will just crash.
  3. You're right, we should lower case that.
src/middleware/thunk.js
+export default function thunkMiddleware({ dispatch, getState }) {
+ return (next) => (action) =>
+ typeof action === 'function' ?
+ action(dispatch, getState) :

This comment has been minimized.

@dbrans

dbrans Jul 1, 2015

Contributor

Looks like thunkMiddleware gives the call-site the ability to look at the current state of the app and dispatch actions based on that state. Did I get that right? When / how would we use this?

@dbrans

dbrans Jul 1, 2015

Contributor

Looks like thunkMiddleware gives the call-site the ability to look at the current state of the app and dispatch actions based on that state. Did I get that right? When / how would we use this?

This comment has been minimized.

@gaearon

gaearon Jul 1, 2015

Collaborator

For example, the action creator might want to exit early if some data is already available in the state. Also useful for dispatching an action and then reading the result (e.g. authentication workflow).

@gaearon

gaearon Jul 1, 2015

Collaborator

For example, the action creator might want to exit early if some data is already available in the state. Also useful for dispatching an action and then reading the result (e.g. authentication workflow).

src/Store.js
+ this.dispatch({ type: '@@INIT' });
+ }
+
+ dispatch(action) {

This comment has been minimized.

@dbrans

dbrans Jul 1, 2015

Contributor

At this point this method is more like a "performAction" than a "dispatch".
The code reads like "perform an action which modifies state. Then notify listeners that state has changed".

@dbrans

dbrans Jul 1, 2015

Contributor

At this point this method is more like a "performAction" than a "dispatch".
The code reads like "perform an action which modifies state. Then notify listeners that state has changed".

This comment has been minimized.

@gaearon

gaearon Jul 1, 2015

Collaborator

I agree in a way. I'm not sure it's worth changing though. It doesn't seem to cause much confusion (unlike “Stores” naming which did, so we're changing it to “Reducers”).

@gaearon

gaearon Jul 1, 2015

Collaborator

I agree in a way. I'm not sure it's worth changing though. It doesn't seem to cause much confusion (unlike “Stores” naming which did, so we're changing it to “Reducers”).

src/Store.js
+
+ const { reducer } = this;
+ this.state = reducer(this.state, action);
+ this.listeners.forEach(listener => listener());

This comment has been minimized.

@dbrans

dbrans Jul 1, 2015

Contributor

The listener will probably turn around and call getState right away. Why not pass the state to the listener?

@dbrans

dbrans Jul 1, 2015

Contributor

The listener will probably turn around and call getState right away. Why not pass the state to the listener?

This comment has been minimized.

@gaearon

gaearon Jul 1, 2015

Collaborator

@dbrans In fact this is how it used to be, but I felt it was redundant because the components would reach into getState() anyway in some cases. So there's no real reason other than making the API do less things. I don't have a strong opinion here though.

@gaearon

gaearon Jul 1, 2015

Collaborator

@dbrans In fact this is how it used to be, but I felt it was redundant because the components would reach into getState() anyway in some cases. So there's no real reason other than making the API do less things. I don't have a strong opinion here though.

This comment has been minimized.

@iclanzan

iclanzan Jul 9, 2015

I really think state should be passed to the listener.

@iclanzan

iclanzan Jul 9, 2015

I really think state should be passed to the listener.

@emmenko

This comment has been minimized.

Show comment
Hide comment
@emmenko

emmenko Jul 1, 2015

Collaborator

@gaearon one step forward, great work! 👍

Collaborator

emmenko commented Jul 1, 2015

@gaearon one step forward, great work! 👍

src/createStore.js
+ return {
+ dispatch: cookedDispatch,
+ subscribe: ::store.subscribe,
+ getState: ::store.getState,

This comment has been minimized.

@iamdustan

iamdustan Jul 1, 2015

Contributor

I’m curious why a second ::store.getState function is created instead of reusing the one from #L20?

@iamdustan

iamdustan Jul 1, 2015

Contributor

I’m curious why a second ::store.getState function is created instead of reusing the one from #L20?

This comment has been minimized.

@gaearon

gaearon Jul 1, 2015

Collaborator

Overlooked this :-). No real reason

@gaearon

gaearon Jul 1, 2015

Collaborator

Overlooked this :-). No real reason

@gaearon gaearon referenced this pull request in cloverfield-tools/universal-react-boilerplate Jul 1, 2015

Open

Roadmap #4

2 of 20 tasks complete
Merge pull request #200 from iamdustan/breaking-changes-test-id-incre…
…menter

Modify test case ID generation for reducer changing
@rpominov

This comment has been minimized.

Show comment
Hide comment
@rpominov

rpominov Jul 1, 2015

I've been wondering if composeReducers() is really the best name for that function.
Maybe we should think of a more specific name? Don't have any suggestions right now, but I wonder what everyone else's thoughts are.

combineReducers() maybe?

rpominov commented Jul 1, 2015

I've been wondering if composeReducers() is really the best name for that function.
Maybe we should think of a more specific name? Don't have any suggestions right now, but I wonder what everyone else's thoughts are.

combineReducers() maybe?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 1, 2015

Collaborator

I like combine.

Collaborator

gaearon commented Jul 1, 2015

I like combine.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 2, 2015

Collaborator

👍 for combineReducers().

I think we should remove middleware as a parameter to createRedux(). Because they have the potential to be async, middleware needs to be applied before being passed to higher-order stores like createDebuggerRedux(): #166 (comment)

Instead, I think the API for middleware should be something like applyMiddleware() from that comment.

I can update this branch with applyMiddleware() if you agree, @gaearon.

Collaborator

acdlite commented Jul 2, 2015

👍 for combineReducers().

I think we should remove middleware as a parameter to createRedux(). Because they have the potential to be async, middleware needs to be applied before being passed to higher-order stores like createDebuggerRedux(): #166 (comment)

Instead, I think the API for middleware should be something like applyMiddleware() from that comment.

I can update this branch with applyMiddleware() if you agree, @gaearon.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 2, 2015

Collaborator

Oh, you're right!

Collaborator

gaearon commented Jul 2, 2015

Oh, you're right!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 2, 2015

Collaborator

@acdlite Please do!

Collaborator

gaearon commented Jul 2, 2015

@acdlite Please do!

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 2, 2015

Collaborator

@gaearon Hmm, just realized... we can't keep thunkMiddleware() as a default if we move middleware out of createStore().

Also, should applyMiddleware() be composable using compose()? E.g.

const store = compose(
  applyMiddleware(t => [thunkMiddleware(t), promiseMiddleware]),
  debugger,
  createStore
)(reducer, intialState)

or should it be applied to the store after it's created? E.g.

const store = applyMiddleware(
  t => [thunkMiddleware(t), promiseMiddleware],
  debugger(createStore)(reducer, initialState)
)

The second version looks nicer and is less likely to confuse people, but the first version is more flexible and "correct" since it has the same interface as debugger.

Collaborator

acdlite commented Jul 2, 2015

@gaearon Hmm, just realized... we can't keep thunkMiddleware() as a default if we move middleware out of createStore().

Also, should applyMiddleware() be composable using compose()? E.g.

const store = compose(
  applyMiddleware(t => [thunkMiddleware(t), promiseMiddleware]),
  debugger,
  createStore
)(reducer, intialState)

or should it be applied to the store after it's created? E.g.

const store = applyMiddleware(
  t => [thunkMiddleware(t), promiseMiddleware],
  debugger(createStore)(reducer, initialState)
)

The second version looks nicer and is less likely to confuse people, but the first version is more flexible and "correct" since it has the same interface as debugger.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 2, 2015

Collaborator

(Hahaha I guess calling it debugger isn't an option :D)

Collaborator

acdlite commented Jul 2, 2015

(Hahaha I guess calling it debugger isn't an option :D)

@Keats Keats referenced this pull request Jul 2, 2015

Closed

Typescript definition #206

acdlite added some commits Jul 4, 2015

Fix composeMiddleware and move back into separate module
Discovered a subtle and confounding bug with composeMiddleware where
it only works if dispatch is the last argument. It did not combine
multiple middlewares into a single middlewares as advertised; it
only combined multiple middlewares with dispatch to create a new
dispatch. This didn't come up earlier because the codebase never
happened to use it in the former way.

This commit fixes the issue and adds a test for it.

@acdlite acdlite referenced this pull request Jul 4, 2015

Merged

Update middleware api #213

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 4, 2015

Collaborator

Submitted a PR to this branch to update middleware API #213

Collaborator

acdlite commented Jul 4, 2015

Submitted a PR to this branch to update middleware API #213

gaearon and others added some commits Aug 6, 2015

Merge pull request #412 from clintwood/jsnext
add jsnext:main in package.json for next gen bundling
Merge pull request #415 from quicksnap/breaking-changes-1.0
Modify package.json description
Merge pull request #439 from kevinold/fix-spawnSync
adjust scoping for opts to avoid error with spawnSync
Merge pull request #453 from hartzis/update-counter-example
Update counter example to recent react-redux
Fixes #461 - Copy listeners array, so subscribes can't affect the loop.
Unsubscribing during a dispatch would change the listeners array.
However that causes the next listener not to fire.
Merge pull request #462 from arian/fix-461-unsubscribing-listeners
Fixes #461 - Copy listeners array, so subscribes can't affect the loop.
Merge pull request #460 from gaearon/add-async-example
Add “real world” example
@idolize

This comment has been minimized.

Show comment
Hide comment
@idolize

idolize Aug 12, 2015

Contributor

Any reason why you don't have extends Component? Does it matter?

Also, here you define propTypes inside the class via static, but in List.js you set them on the class at the bottom of the file (outside the declaration). Is there any reason for the inconsistency?

Any reason why you don't have extends Component? Does it matter?

Also, here you define propTypes inside the class via static, but in List.js you set them on the class at the bottom of the file (outside the declaration). Is there any reason for the inconsistency?

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 12, 2015

Collaborator

Lazy copy paste, I should fix that. We don't use ES next proposals in this repo.

Extending Component isn't currently required unless you need setState but I should add that too because people ask about it all the time. :-)

Collaborator

gaearon replied Aug 12, 2015

Lazy copy paste, I should fix that. We don't use ES next proposals in this repo.

Extending Component isn't currently required unless you need setState but I should add that too because people ask about it all the time. :-)

danmartinez101 and others added some commits Aug 12, 2015

Merge pull request #466 from danmartinez101/use-es6-in-real-world-exa…
…mple

Update real world example to remove ES7
Merge pull request #467 from nbostrom/fix-object-assign-in-example
Fix syntax error in Object.assign in real world example
Real World Example - Fix typo in API middleware error string
This changes 'on' to 'one' in the string 'Specify on of the exported Schemas.'
Merge pull request #471 from HPate-Riptide/patch-1
Real World Example - Fix typo in API middleware error string

gaearon added a commit that referenced this pull request Aug 14, 2015

@gaearon gaearon merged commit 6d0107e into master Aug 14, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 14, 2015

2.5 (insanely hectic for you)months from "Initial Commit" to "1.0" is kind of crazy... Nice work :)

ghost commented Aug 14, 2015

2.5 (insanely hectic for you)months from "Initial Commit" to "1.0" is kind of crazy... Nice work :)

@gaearon gaearon deleted the breaking-changes-1.0 branch Aug 14, 2015

@teameh

This comment has been minimized.

Show comment
Hide comment
@teameh

teameh Mar 20, 2016

Why are these consts only used here and not in the reducers? The api middleware looks really nice but is not used by it full extend if the reducers don't use these consts here to differentiate between different actions right? The reducers just check if the action contains a response and and entities which works. But is not showing off the full potential of and and usage of the middleware isn't it? I've found this a bit confusing when I was trying to comprehend the example.

Why are these consts only used here and not in the reducers? The api middleware looks really nice but is not used by it full extend if the reducers don't use these consts here to differentiate between different actions right? The reducers just check if the action contains a response and and entities which works. But is not showing off the full potential of and and usage of the middleware isn't it? I've found this a bit confusing when I was trying to comprehend the example.

This comment has been minimized.

Show comment
Hide comment
@teameh

teameh Mar 20, 2016

Great example btw 😀 !

Great example btw 😀 !

@Igor10k

This comment has been minimized.

Show comment
Hide comment
@Igor10k

Igor10k Jan 28, 2017

If this action will lead to an error in one of the components the error will be swallowed by the promise and the error stack will be lost. Just ran into that and still looking for a way to render such error in the console taking into the account the source map.

If this action will lead to an error in one of the components the error will be swallowed by the promise and the error stack will be lost. Just ran into that and still looking for a way to render such error in the console taking into the account the source map.

This comment has been minimized.

Show comment
Hide comment
@teameh

teameh Jan 29, 2017

Yeah I always struggle with that as well..

Yeah I always struggle with that as well..

This comment has been minimized.

Show comment
Hide comment
@Igor10k

Igor10k Jan 29, 2017

Changed that block to

error => {
  error instanceof Error && console.error(error)

  return next(actionWith({
    type: failureType,
    error: error.message || 'Something bad happened'
  }))
}

In combination with config.devtool = 'inline-source-map' in Webpack I finally have errors with source mapped stack trace in console. Yey!

Changed that block to

error => {
  error instanceof Error && console.error(error)

  return next(actionWith({
    type: failureType,
    error: error.message || 'Something bad happened'
  }))
}

In combination with config.devtool = 'inline-source-map' in Webpack I finally have errors with source mapped stack trace in console. Yey!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment