redux-saga / redux-saga

An alternative side effect model for Redux apps
https://redux-saga.js.org/
MIT License
22.54k stars 1.97k forks source link

Consider adding `safe` effect wrapper #1250

Open Andarist opened 6 years ago

Andarist commented 6 years ago

If somebody wants to work in it - let me know. We can then establish how this would work

madhums commented 6 years ago

@Andarist could you describe what is the idea behind "safe" effect?

Andarist commented 6 years ago

Im not yet exactly sure about the API, but one idea is to:

const safe = effect => (effect[SAFE] = true)

function* someSaga() {
  // where result is of type Error or Result
  const result = yield safe(call(someAPI, arg1))
  // in case of an error would die gracefully instead of failing the parent 
  yield safe(fork(otherSaga, arg2))
}

In this variant SAFE would have to be interpreted in the redux-saga's core.

Other way of implementing it would be require implementing equivalent of:

function* safe(effect) {
    try {
        return { result: yield effect }
    } catch (err) {
        return { err }
    }
}

In this variant fork would have to be treated differently, so the task descriptor could get returned to the caller, instead of blocking.

SeregaSE commented 6 years ago

I can try do this

Andarist commented 6 years ago

Cool!

Seems to be that second variant of what I have proposed (no SAFE symbol, but rather just a simple wrapper around yield effect) is what we should implement.

mshustov commented 6 years ago

I'm using next pattern in my projects, that's kinda 2nd version

function* safe(effect) {
  try {
    return { result: yield effect, error: null }
  } catch (error) {
    return { result: null, error }
  }
}
younesmln commented 6 years ago

@Andarist interested in working on this enhancement

Andarist commented 6 years ago

@younesmln go ahead, if you need any help - please just ask

jaulz commented 5 years ago

@restrry how do you use it? I tried something like that:

sagas.map((saga) => safe(spawn(saga)))

but still only the top level error handler is called and not the specific of the safe function.

mshustov commented 5 years ago

@jaulz I think the problem in the snippet that it uses spawn

The parent will not wait for detached tasks to terminate before returning and all events which may affect the parent or the detached task are completely independents (error, cancellation).

https://github.com/redux-saga/redux-saga/tree/master/docs/api#spawnfn-args

could you try to use fork instead?

fgfmichael commented 4 years ago

I have tried to implement via @restrry solution however I get the following error:

Error: "fork: argument of type {context, fn} has undefined or null `fn`"

this happens when i try to use it at a core level, basically my goal being to wrap my entire process:

export default function* rootSaga() {
  yield all([
    takeLatest(actions.CREATE_FILE_UPLOAD, safe(fork(uploadFile))),
  ]);
}

I have also tried without fork and even though it felt wrong I've also tried wrapping the takeLatest instead plus also i've tried not using fork given that takeLatest already does this (just tryin anything). All don't work and I'm pretty sure had the basically the same error message.

Perhapse you could explain why we return { result, error} if there is one so I can attempt to understand the underlying functionality? Also maybe I'm just using this wrong?

For now I am just going to wrap the code I know is likely to crash in a try catch, my main reason for looking into this however is that I don't plan on ever actioning the crash (its fine if this happens) so I would rather enrich my saga via composition instead of having to dump code into a working function.

vespaiach commented 3 years ago

@fgfmichael the idea is: don't yield your effects immediately inside your function, let's another "safe" function do it.

Example:

function* safe(effect) {
  try {
    return { response: yield effect }
  } catch (error) {
    return { error }
  } 
}

function* fetchProducts() {
  const { response, error } = yield safe(call(Api.fetch, '/products'))
  if (response)
    yield put({ type: 'PRODUCTS_RECEIVED', products: response })
  else
    yield put({ type: 'PRODUCTS_REQUEST_FAILED', error })
}
yash431garg commented 1 year ago

Hi, Is anyone working on this issue @Andarist?

Kirill486 commented 12 months ago

Hi, everyone)) I'm honored to take this issue. I already have some code written for exactly the same purpose)

2320

Kirill486 commented 12 months ago

Hey, @Andarist ) I've created a draft pull request for this issue #2392 . It contains a few red tests on how this feature could've worked if it was implemented. Its a little different from what was discussed in this issue. Please take a look, I want your opinion on that. After we reach an agreement on the api in tests, the implementation itself is a piece of cake)))