tp / tsaga

Typesafe and lightweight way to write Redux-connected functions with asynchronous side-effects that are easily testable.
Apache License 2.0
8 stars 1 forks source link

Feature/effect #22

Closed tp closed 5 years ago

tp commented 5 years ago

cc @HenriBeck

The change in here is straightforward, but but the usage in app is getting verbose again:

const executeBasketCallAndUpdateBasketAsync = withEnv(
  async (
    {dispatch, select, run, fork}: OmsEnv,
    bapiRequest: BapiCall<BasketResponseData>
  ): Promise<BasketResponseData> => {

Mostly annoying to me is the inner function declaring and the fact that we either loose the function name or would have to repeat it. So in tests we can't put a nice log when no mock is found for a given function.

HenriBeck commented 5 years ago

We can access the function name from function.name. This even works for named arrow functions. I think this will make it easier to use. The only thing which is a little bit complicated with that are forks I think.

I don't think the syntax is too verbose. It makes executing functions inside a saga much easier.

tp commented 5 years ago

This PR still has the issue with type inference, as run(callApi, requestResponseObject) breaks the type inference.

Initially I worked around this by creating a different type of effect / effect creator, which would need to be run with the env and contain the proper type inside itself.

But the downside would be that the call-site would have to written like this:

run(callApi(requestResponseObject))

Which in itself isn't bad (I would even prefer it to the normal run), but the issue I have with this is inconsistency between a normal function and one that returns an effect. (At least as long as the same run is used for both.)

HenriBeck commented 5 years ago

Yeah, this will also make it hard to mock effects in testing.

tp commented 5 years ago

True. I haven't been able to come up with a pattern that allows for correct type-inference and being testable as well.

They would all rely on the user using the correct helper functions, such that the initial function and parameters are saved for comparison in tests. (But it's very easy to just depend on a parameter via closure from the outer "generator" function, which then can't be tested for.)

Overall it's just not very nice to use in the application code.

I am wondering whether we should just use env explicitly, and then we could use run(f, env) and it would be implicit that f is a child with full access to the store (one of my big dislikes about redux-saga, that one can't identify how complex a child is and whether it can cause store updates at first glance).

being able to write helpers like this (with deferred env injection) would help with correct type inference, but I see no way to (mostly) guarantee that we collect the function and arguments for testing: image

HenriBeck commented 5 years ago

Check out #23

Type inference is working for me there.

HenriBeck commented 5 years ago

True, the issue about not knowing what this function actually might do with the store is there, but I feel like that's also the same take for example.

tp commented 5 years ago

work in progress: image

That way callBapi123 returns the proper type based on the parameter.

Functions that don't accept an env as first parameter / return an EffectCreator fail to type never.

The implementation and overwriting a prop on the function is not the nicest, but at least it would fix the usage.

TODO: in the id function return a custom function that captures/exposes the parameters, so we can compare them in testing.