search

joshburgess / redux-most

Most.js based middleware for Redux. Handle async actions with monadic streams & reactive programming.
https://www.npmjs.com/package/redux-most
MIT License
139 stars 14 forks source link

Add support for injecting a custom argument into epics #26

Open alex2 opened 6 years ago

alex2 commented 6 years ago

Hi, after using the project for a bit I'm really missing the dependency injection from thunk and redux-observable for my testing.

Implementing this was raised in issue #19 but dismissed at the time in favour of a more elegant HOF approach and imminent API changes, but after four months with no project activity I've gone for the simplest, most pragmatic implementation that I believe is backwards-compatible.

If you're willing to accept this (until the lovely proposed HOF-support rewrite arrives) let me know and I'll update the documentation too.

alex2 commented 6 years ago

(there's some travis config issue - the tests should pass!)

joshburgess commented 6 years ago

Hi, @alex2 . I will check this out very soon. Unfortunately, I've been pretty busy lately. I've been meaning to get back to this and update the project to use most/core, include Flow types, explore the HOFs & this dependency injection feature. I just haven't had time recently. Thanks for submitting the PR. I'd love more help, in general!

I'm currently interviewing for new jobs and very busy... I have some take home work to complete this weekend, but I may have a bit of time free if I finish it soon enough. Otherwise, it will be soon after. Sorry for the wait.

alex2 commented 6 years ago

No worries, really good of you to let me know. Good luck with your interviewing!

joshburgess commented 6 years ago

@alex2 After reviewing, yes, I am willing to accept this.

Here are the remaining tasks that need to be accomplished:

  1. As you said, we need to update the documentation to reflect the API change.

  2. The only other thing than documentation that you missed is updating the two Epic interfaces & the createEpicMiddleware function in the index.d.ts TypeScript type definitions file. This should be relatively straight forward, given what is already there, but let me know if you'd like me to do it instead. Unfortunately, I believe we will have to use the any type for the dependencies argument, as it's really up to the user what they want to pass in there.... it could be a function, an object, etc...

  3. I'd like to merge this and all future PR requests into the dev branch. I manually test things out there to make sure all is safe before merging into master.

Let me know if you're okay with tackling these things. I'm okay with doing any or all of them if you'd prefer, but I figured I'd leave it to you if you'd like to contribute.

Thanks again for taking the initiative to make a PR for this! I had been neglecting the project for a bit too long.

joshburgess commented 6 years ago

@alex2 You still interested in helping finish this up (docs + type definitions) or would you like me to merge as is and help finish it?

alex2 commented 6 years ago

Hey, I've also been interviewing for a new job(!) but managed to be much less communicative than you while doing it, so my apologies for the lack of updates. I'll have some free time this weekend to make your suggested changes.

joshburgess commented 6 years ago

@alex2 You should do a pull. The type definitions file recently changed somewhat due to someone's pull request to make it compatible with Redux 4.0. The changes are minor, but there are some conflicts right now.

FWIW, I'll wait until later with this, but I've been thinking about changing the API soon to just use object destructuring to pass all params at the same time as one object arg.

Like ({ dispatch, getState, dependencies, stateStream }) => ..., etc.... We should wait on this until later, but what do you think?

It makes a point-free style harder/less readable in some cases, but removes the positional aspect of the arguments and lets you just treat it as a grab-bag, letting you grab the things you need and ignore the rest.