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

Added explicit dependencies option to createEpicMiddleware #193

Merged
merged 3 commits into from Mar 2, 2017

Conversation

Projects
None yet
3 participants
@bali182
Contributor

bali182 commented Feb 11, 2017

Edit: read about this new feature here: https://redux-observable.js.org/docs/recipes/InjectingDependenciesIntoEpics.html


Implements #163

@bali182

This comment has been minimized.

Contributor

bali182 commented Feb 11, 2017

@jayphelps let me know if I should add more tests or change something!

@jayphelps

This comment has been minimized.

Member

jayphelps commented Feb 11, 2017

Wow awesome!! Impressed with the test coverage actually including the discussed combineEpics spread support.

Everything looks great, we just gotta decide if defaulting to null is the right choice instead of not passing anything at all. Any arguments either way?

@bali182

This comment has been minimized.

Contributor

bali182 commented Feb 11, 2017

Of course, I wasn't sure about null either, it could be undefined as well. I didn't really want to do anything opinionated like an empty object or array, but I'm open to change it to any other default value.

Glad you liked the tests, tried to cover everything I can think of!

@BerkeleyTrue

This comment has been minimized.

Collaborator

BerkeleyTrue commented Mar 1, 2017

@bali182 Nice work. I'd definitely think leaving it undefined is a better option.

@jayphelps should the function avoid passing in the third option if it undefined?

@jayphelps jayphelps force-pushed the bali182:master branch from 777fe20 to 5296754 Mar 1, 2017

@jayphelps jayphelps force-pushed the bali182:master branch from 5296754 to 8371c7c Mar 1, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 1, 2017

@BerkeleyTrue I believe so, that way arguments.length is not 3 (which it is even if you pass undefined literally.) Kinda a pain, but I think it's the "right thing to do".

I've amended the PR with those changes. Can someone give a final review before I merge?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 1, 2017

Actually, I think we should be a stickler about not merging this until it includes documentation, even if brief. I think another section under Basics, something like "Injecting Dependencies Into Epics" but @bali182 might have better suggestions?

@bali182

This comment has been minimized.

Contributor

bali182 commented Mar 1, 2017

@jayphelps Sounds good as a title, roughly that's how I googled it and found the open issue :)

If you want I could help with the docs, have some free time this afternoon.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 1, 2017

@bali182 please do! can just amend this PR

@bali182

This comment has been minimized.

Contributor

bali182 commented Mar 2, 2017

@jayphelps Done! Please do look at createEpicMiddleware.md with extreme care, because I added some description about the options argument, and I'm not 100% sure you planned it this way. Let me know I should change it anywhere!

@jayphelps jayphelps force-pushed the bali182:master branch from 463d736 to e83a273 Mar 2, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 2, 2017

@bali182 awesome. I made some changes, can you give me your honest thoughts? 1ac1d0e

@jayphelps jayphelps force-pushed the bali182:master branch 4 times, most recently from ef71ee5 to 1ac1d0e Mar 2, 2017

type: 'FETCH_USER_FULFILLED',
payload: response
}))
);

This comment has been minimized.

@jayphelps

jayphelps Mar 2, 2017

Member

I mostly changed this to fetching a single user (simpler as an example to grok) and adjusted some things to be consistent with the other doc examples like mergeMap vs flatMap, isolating the observable chains by putting the map inside the mergeMap.

@bali182

This comment has been minimized.

Contributor

bali182 commented Mar 2, 2017

@jayphelps Read the whole thing, thanks for correcting it (especially the Observable => any and any => Observable, this is exactly why I asked to take a look :)! The recipe looks much clearer, having an actual example of how things can be mocked helps a lot. Also it helps that the wording is coming from a native English speaker. I'd say it's good. Only thing I don't understand is that why mergeMap is used in all examples. Coming from java/scala flatMap is much more self explanatory for me but that's just personal preference.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Mar 2, 2017

@bali182 thanks for reading.

Some backstory: mergeMap is preferred because that's what RxJS v5 prefers. The documentation, links, filenames, etc are all mergeMap, only referencing that flatMap is an alias. The justification of that change in RxJS is obviously not redux-observable related, but was primary because we discussed the naming of flatMap + flatMapLatest with the community and by and large people (who were not already familiar with the term flatMap) preferred mergeMap and that it was quicker and easier to understand what it did. It also made it more consistent with the other merge operators, which in other Rx implementations are inconsistent. That said...that decision was made like almost 2 years ago and since then casually discussed the possibility of going back to flatMap but sticking with switchMap (instead of flatMapLatest). But I wouldn't bet on it changing.. For now we just go with the canonical name RxJS v5 uses.

@bali182

This comment has been minimized.

Contributor

bali182 commented Mar 2, 2017

@jayphelps thanks for the explanation! Didn't know about the mergeMap preference in rxjs

wip

@jayphelps jayphelps force-pushed the bali182:master branch from 1ac1d0e to fe082be Mar 2, 2017

@jayphelps jayphelps merged commit 7e2a479 into redux-observable:master Mar 2, 2017

1 check passed

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

This comment has been minimized.

Collaborator

BerkeleyTrue commented Mar 2, 2017

Noice!

@jayphelps

This comment has been minimized.

@bali182

This comment has been minimized.

Contributor

bali182 commented Mar 3, 2017

Great! 🎉 🎉 🎉

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