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

bugfix: pass along all actions to next middleware in chain #5

Merged
merged 4 commits into from
Feb 16, 2017

Conversation

jebeck
Copy link
Contributor

@jebeck jebeck commented Feb 16, 2017

Here it is at last, the fix for #4! Thanks again for giving me the nudge @c-h-, this has been in the back of my mind to finish up for so long.

There were actually two bugs/exceptions to the general middleware pattern going on before:

  1. An action with the Worker properties didn't get passed along to any following middlewares in the chain.
  2. An action returned from the Worker was being passed along the chain with next instead of being dispatched as a new action, so it was only passing through the following links in the middleware chain rather than all of them; this had the potential to be a very large bug for anyone trying to dispatch some kind of a fetch request (or anything needing to be picked up by a middleware at the top of the chain) via the Worker's response.

I had to access the dispatch from the store at the top level of the layers of function currying in order to fix the second issue, so I added some testing around that as well, naturally. This resulted in problems with the setTimeout(fn, 0) strategy being used in the Worker polyfill, so I've updated the polyfill strategy a bit in a way that I believe makes sense, and I had to use setTimeout in some of the tests. (Alternatively mock timers could be used in the tests, but I don't think expect has that functionality? I'm more accustomed to working with Chai + Sinon myself.) The tests and modifications to the polyfill are probably where I'd spend the most time reviewing, to make sure I didn't miss anything!

Probably not necessary, but just to illustrate how these changes had an effect on an existing app. Here is the redux-logger operating after the worker middleware in the chain using the current version of redux-worker-middleware (without my fixes):
screenshot 2017-02-15 16 35 41

Note here that there are no DATA_PROCESS_REQUEST actions logged (those are the ones with the Worker meta properties that get dropped from the chain). The DATA_PROCESS_SUCCESS are the actions returned by the Worker.

Now here is the same fetch and data processing sequence when I locally npm link my clone of the redux-worker-middleware with the fixes:

screenshot 2017-02-15 16 40 35

Observe that we have two FETCH_DATA_SUCCESS, which each trigger a DATA_PROCESS_REQUEST (the action with Worker meta properties, which now we see logged instead of dropped), and also the DATA_PROCESS_SUCCESS are still there (though now from dispatch to the top of the middlewares chain rather than just going through the remainder of the chain with next).

(Ignore all the FETCH_DATA_SPAN and INITIALIZE_DOMAIN stuff intervening in there...those requests were slower in the first screenshot because the server hadn't cached them yet...😛 Also the dispatching of the Worker-generated result action to the top of the chain may be changing the sequence of things happening/getting logged. There's a lot of async data fetching and processing going on in this app.)

@c-h-
Copy link

c-h- commented Feb 16, 2017

Woo hoo!! excited to use it

@keyz keyz merged commit 7aff178 into keyz:master Feb 16, 2017
@keyz
Copy link
Owner

keyz commented Feb 16, 2017

Awesome, thank you @jebeck! I'll then clean up some outdated deps and publish a new major.

@jebeck
Copy link
Contributor Author

jebeck commented Feb 16, 2017

You read my mind @keyanzhang, I was going to suggest taking a look at the deps! 👍

keyz added a commit that referenced this pull request Feb 20, 2017
- We used to let the worker pass processed actions to `next`, which was a bug. Processed actions should be passed to `dispatch` instead (see #5). Since `worker.onmessage` no longer needs `next`, we can move it up a bit.
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.

None yet

3 participants