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

[Feature request] Inject dependencies in epics #163

Closed
AugustinLF opened this Issue Dec 20, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@AugustinLF

AugustinLF commented Dec 20, 2016

In my app when I don't use redux-observable, I use a variant of redux-thunk that lets me inject dependencies in my actions (see how it is used in this action), it's also something made possible by the redux-inject middleware. This lets me have efficient DI in my actions, depencencies set up at the store creation. However, redux-observable doesn't provide such an API.

For instance, it would work like that

const store = createStore(
  rootReducer,
  applyMiddleware(epicMiddleware, dependencies),
);

Would that be something you would be interested? If so, I'd gladly work on a PR.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 20, 2016

Are you wanting to inject things into the actions themselves or inject things into your epics?

I feel pretty confident we won't support injecting things into the actions themselves--it's really outside the scope of redux-observable and easily done with other middleware could would use addition. I also personally don't think people should do that, but want to stress I'm talking about POJO actions, not thunks (which aren't technically actions, but sometimes colloquially called "async actions" but that's a bit of a misnomer).

For injecting dependencies into the epics, this is already supported as a pattern, but no first-class API exists for it yet. See #115

// given epics like this
const epic1 = (action$, store, api) => {
  // access to API here!
};
import * as api from './api/location';

// inject API into all epics as third arg
const rootEpic = (...args) => combineEpics(epic1, epic2)(...args, api);

which makes it easier to test:

const fakeApi = {
  fetchSomething: () => Observable.of('fake response')
};
epic1(action$, fakeStore, fakeApi);

I'm not immediately opposed to to making this pattern first-class, something like this:

// HYPOTHETICAL ONLY
createEpicMiddleware(rootEpic, { dependencies: [api] });
@AugustinLF

This comment has been minimized.

AugustinLF commented Dec 22, 2016

Thanks for your answer, I indeed meant injecting things into the epics! I didn't think of your workaround, that would do the job, great :)

But your proposal is what I had in mind.

@bali182

This comment has been minimized.

Contributor

bali182 commented Feb 10, 2017

If this is implemented, how would additional arguments be handled? Would the config objects dependencies field simply overtake the role of passing more stuff into epics or would the current way of passing arguments kept, and dependencies passed after what is currently passed?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Feb 10, 2017

@bali182 if you mean would this replace

const rootEpic = (...args) => combineEpics(epic1, epic2)(...args, api);

yes, it would. You could still do this, but it would be bumped to the fourth arg position instead of the third, as the third would be anything injected using the dependencies option. If you don't inject anything using dependencies, I'm not sure if we would choose to either force null or undefined, or just not pass anything at all, freeing up that parameter position. If we don't pass anything things might be confusing that this sort of spread logic won't always result in your injection being in the fourth position. On the other hand, forcing it to be null or undefined is wasteful and annoying.

The primary reason I haven't added this is that I've been hesitant to lay claim on that parameter position, cause I wasn't sure if we would need it for something more important. But it's been over a year and I'm pretty happy with the status quo, so adding this before 1.0 seems like a good idea. (along with a testing docs overhaul with learnings)

@bali182

This comment has been minimized.

Contributor

bali182 commented Feb 10, 2017

@jayphelps Great, that's good news! So dependencies will be a single value passed in epics at a fixed argument position? The example with { dependencies: [api] } gave me the impression, that you want to pass this spread style as well (thinking of the array) but I like the single-value-at-fixed-position version much better. You can always pass an object and destructure in the epic this way, and it's harder to shoot yourself in the foot by messing with argument order.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Feb 10, 2017

@bali182 oh I misunderstood cause I forgot that I proposed an array of dependencies.

I'm leaning instead towards { dependencies: someobject }, so someobject which be passed as-is as the third argument, no spreading--which I think is what you want as well.

createEpicMiddleware(rootEpic, { dependencies: api });

// later

const epic1 = (action$, store, api) => {
  // access to API here!
};

Or you can do other things like this:

createEpicMiddleware(rootEpic, { dependencies: { api, somethingelse } });

// later

const epic1 = (action$, store, { api, somethingelse }) => {
  // access to API and somethingelse 
};

No matter what, it's only third arg.

@bali182

This comment has been minimized.

Contributor

bali182 commented Feb 10, 2017

Yes this sounds perfect in my opinion. Also not much work to migrate if someone was relying on the extra arguments.

bali182 added a commit to bali182/redux-observable that referenced this issue Feb 11, 2017

bali182 added a commit to bali182/redux-observable that referenced this issue Feb 11, 2017

jayphelps added a commit to bali182/redux-observable that referenced this issue Mar 1, 2017

jayphelps added a commit to bali182/redux-observable that referenced this issue Mar 1, 2017

@jayphelps jayphelps closed this in 7e2a479 Mar 2, 2017

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