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

Change side effect dispatch model #9

Closed
wants to merge 12 commits into from
50 changes: 50 additions & 0 deletions src/Task.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { isUndefined, defer, clearDefer } from './utils';

/**
* A utility class to manage a single defferable task.
*
* @class Task
*/

export default class Task {
_id;

/**
* Checks if a task is set to execute in the future.
*
* @returns {boolean} True if a task is set.
*/

isSet() {
return !isUndefined(this._id);
}

/**
* Defers a function to be executed on the next turn of the event loop.
* Will not allow a defer if the task is already set to execute.
*
* @param {Function} Callback to execute.
*/

defer(callback) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a production code: I don't like passing the callback, we'd better find another approach using Promises.

Copy link
Author

Choose a reason for hiding this comment

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

For deferring? A promise represents the completion of some task. I think things like deferring and setTimeouts make more sense as a callback. For one they should never error. In this case a callback will also be slightly more performant.

if (this.isSet()) {
throw new Error('Cannot set a new task while another is currently set.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a production code: This should become an invariant.

Copy link
Author

Choose a reason for hiding this comment

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

🎉

}

this._id = defer(() => {
this.reset();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this supposed to be this.clear() ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, oops 😊
Again I wanted to get the basic implementation out there so some of the simple stuff slipped through.

callback();
});
}

/**
* Clears the task this class is managing. If the task has not run,
* it will never run. If the task has run, the class will be opened
* up for a new callback to be deffered.
*/

clear() {
clearDefer(this._id);
delete this._id;
}
}
123 changes: 61 additions & 62 deletions src/createEffectCapableStore.js
Original file line number Diff line number Diff line change
@@ -1,60 +1,5 @@
import { invariant, isFunction } from './utils';
import enhanceReducer from './enhanceReducer';
import AppStateWithEffects from './AppStateWithEffects';

/**
* Wraps Store `getState` method. Instead of returning just plain old state,
* it's assumed that the returned state is instance of `AppStateWithEffects`,
* therefore it proxies the call to `getAppState`.
*
* @param {Object} Original Store
* @returns {Function} Wrapped `getState`
*/
export const wrapGetState = store => () => {
const state = store.getState();

if (state instanceof AppStateWithEffects) {
return state.getAppState();
} else {
return state;
}
};

/**
* Wraps Store `dispatch` method. The original `dispatch` is called first, then
* it iterates over all the effects returned from the `getState` function.
* Every effect is then executed and dispatch is passed as the first argument.
*
* @param {Object} Original Store
* @returns {Function} Wrapped `dispatch`
*/
export const wrapDispatch = store => action => {
// Let's just dispatch original action,
// the original dispatch might actually be
// enhanced by some middlewares.
const result = store.dispatch(action);

const effects = store.getState().getEffects();

invariant(effects.every(isFunction),
`It's allowed to yield only functions (side effect)`);

// Effects are executed after action is dispatched
effects.forEach(effect => effect(wrapDispatch(store)));

return result;
};

/**
* Wraps Store `replaceReducer` method. The implementation just calls
* the original `replaceReducer` method but the provided next reducer
* is enhanced.
*
* @param {Object} Original Store
* @returns {Function} Wrapped `replaceReducer`
*/
export const wrapReplaceReducer = store => nextReducer =>
store.replaceReducer(enhanceReducer(nextReducer));
import Task from './Task';
import createEnhanceReducer from './createEnhanceReducer';

/**
* Creates enhanced store factory, which takes original `createStore` as argument.
Expand All @@ -68,13 +13,67 @@ export const wrapReplaceReducer = store => nextReducer =>
* @returns {Function} Store factory
*/
export default createStore => (rootReducer, initialState) => {
const store = createStore(enhanceReducer(rootReducer), new AppStateWithEffects(initialState, []));
// Reference to the store dispatch function.
let dispatch;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a production code: We would need to get rid of this mutable reference, maybe some thunk could help

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how I would make this into a thunk 😖


const cleanTask = new Task();
const effectQueue = [];

/**
* Takes all of the effects in the closure effect queue, dispatches them,
* captures their values and empties the queue.
*
* @private
* @returns {Array<any>} The return values of dispatch.
*/

const cleanEffectQueue = () => {
const values = effectQueue.map(dispatch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As explained:

  1. It should definitely be banned to dispatch an action here, only thunks should be allowed otherwise we would allow user to cascade actions.
  2. We can't afford reliance on user's redux-thunk as peer dependency

effectQueue.length = 0; // @see https://davidwalsh.name/empty-array
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a production code: Yes agreed, yet this needs to be hidden in some function to make it more clear.

Copy link
Author

Choose a reason for hiding this comment

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

Move the array truncation to a util function?

Copy link
Author

Choose a reason for hiding this comment

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

I am a bit confused by this, should the array truncation be "hidden" or should the "cleanEffectQueue" function be put in the global file scope?

return values;
};

/**
* Defers an effect to be dispatched by `cleanEffectQueue` later. Does this by
* adding the effect to an internal queue and deferring the `cleanEffectQueue`
* function if no other task is set.
*
* @private
* @param {any} The effect action to be dispatched later.
*/

const deferEffect = effect => {
effectQueue.push(effect);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a production code: We'd better move the function off the closure and pass the effect queue, probably with some more immutable API (spread)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why we would want to move this out of the closure. It seems like a lot more code in currying then is necessary. Plus redux uses this pattern.


if (!cleanTask.isSet()) {
cleanTask.defer(cleanEffectQueue);
}
};

/**
* Dispatches an action object, like normal, but also captures the result of effects
* to return to the user. This allows a user to use `Promise.all` or other await
* mechanisms for server side data prefetching.
*
* @param {any} The action to dispatch.
* @returns {Array<any>} The result of all of the dispatched actions and the result of the main action as the last item in the array.
*/

const dispatchReturnEffects = action => {
const mainEffect = dispatch(action);
return cleanEffectQueue().concat([mainEffect]);
};

// Create the reducer enhancer.
const enhanceReducer = createEnhanceReducer(deferEffect);

// Create the store and set the `dispatch` reference to the store dispatch method.
const store = createStore(enhanceReducer(rootReducer), initialState);
dispatch = store.dispatch;

return {
...store,
dispatch: wrapDispatch(store),
getState: wrapGetState(store),
liftGetState: () => store.getState(),
replaceReducer: wrapReplaceReducer(store)
replaceReducer: nextReducer => store.replaceReducer(enhanceReducer(nextReducer)), // TODO: This could be a good usecase for a compose function?
Copy link
Collaborator

Choose a reason for hiding this comment

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

indeed

dispatchReturnEffects
};
};
56 changes: 56 additions & 0 deletions src/createEnhanceReducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import {
isFunction,
isUndefined,
isIterable,
invariant,
mapIterable
} from './utils';

/**
* AppStateOrEffect mapper - extracts just the value.
*
* @param {Object} The generator iteration object
* @returns {any} Might be either Function (for Effect) or any for Application State
*/
const mapValue = iteration => iteration.value;

/**
* Creates a reducer enhancer which iterates over generator-like reducer reduction and
* dispatches all the side effects while returning application state.
*
* @param {Function} A dispatch function which is deffered to after call stack completion.
* @returns {Function} A reducer enhancer which takes the old reducer and returns a new one.
*/

export default function createEnhanceReducer(defferedDispatch) {
return rootReducer => (state, action) => {
invariant(isFunction(rootReducer),
`Provided root reducer is not a function.`);

// Calling the Root reducer should return an Iterable
const reduction = rootReducer(state, action);

if (isIterable(reduction)) {
// Iterable returned by Root reducer is mapped into array of values.
// Last value in the array is reduced application state all the other values
// are just side effects.
const effects = mapIterable(reduction, mapValue);
const newState = effects.pop();

// Dispatch each effect later.
effects.forEach(defferedDispatch);

invariant(!isUndefined(newState),
`Root reducer does not return new application state. Undefined is returned`);

return newState;
} else {
console.warn(
'createEffectCapableStore enhancer from redux-side-effects package is used, ' +
'yet the provided root reducer is not a generator function'
);

return reduction;
}
};
}
85 changes: 0 additions & 85 deletions src/enhanceReducer.js

This file was deleted.

2 changes: 1 addition & 1 deletion src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export const mapIterable = (iterable, mapper) => {
// return the last value in iteration loop
const recur = acc => {
const next = iterable.next();
acc.push(mapper(next.value, next.done));
acc.push(mapper(next));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely! I say yes.


// ES6 tail call
return next.done ? acc : recur(acc);
Expand Down