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

Consider adding safe effect wrapper #1250

Open
Andarist opened this issue Nov 5, 2017 · 14 comments · May be fixed by #2392
Open

Consider adding safe effect wrapper #1250

Andarist opened this issue Nov 5, 2017 · 14 comments · May be fixed by #2392

Comments

@Andarist
Copy link
Member

Andarist commented Nov 5, 2017

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

@Andarist Andarist added this to the v1 milestone Nov 5, 2017
@Andarist Andarist modified the milestones: v1, post-v1 Nov 8, 2017
@madhums
Copy link

madhums commented Nov 12, 2017

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

@Andarist
Copy link
Member Author

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

I can try do this

@Andarist
Copy link
Member Author

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

mshustov commented Jul 26, 2018

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

@Andarist interested in working on this enhancement

@Andarist
Copy link
Member Author

Andarist commented Sep 4, 2018

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

@jaulz
Copy link

jaulz commented Mar 18, 2019

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

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

fgfmichael commented Mar 24, 2020

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

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

Hi, Is anyone working on this issue @Andarist?

@Kirill486
Copy link

Kirill486 commented Sep 25, 2023

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

@Kirill486 Kirill486 linked a pull request Sep 25, 2023 that will close this issue
@Kirill486
Copy link

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)))

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

Successfully merging a pull request may close this issue.

10 participants