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

Add support for injecting a custom argument into epics #26

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

alex2
Copy link

@alex2 alex2 commented Feb 9, 2018

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.

Raised in issue joshburgess#19 but dismissed in favour of a more elegant HOF
approach, after four months with no project activity, I've gone for
the zero-impact pragmatic solution
@alex2
Copy link
Author

alex2 commented Feb 9, 2018

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

@joshburgess
Copy link
Owner

joshburgess commented Feb 9, 2018

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
Copy link
Author

alex2 commented Feb 9, 2018

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

@joshburgess
Copy link
Owner

joshburgess commented Feb 17, 2018

@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.

@alex2 alex2 changed the base branch from master to dev February 17, 2018 22:07
@joshburgess
Copy link
Owner

@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
Copy link
Author

alex2 commented May 2, 2018

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
Copy link
Owner

joshburgess commented May 11, 2018

@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.

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

2 participants