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

It would be better if allSettled will return results of effect execution #385

Closed
bacher opened this issue Sep 21, 2020 · 8 comments
Closed
Labels
core effector package enhancement New feature or request question Further information is requested

Comments

@bacher
Copy link

bacher commented Sep 21, 2020

I'm trying to write SSR Isomorphic application (with Next.js) and I start using Fork api (scopes).

As I could understand (maybe wrong) I should use allSettled func when I use effects on backend-side.
But If I wrap effects into allSettled I'm loosing my ability to read results of effect execution (returning values). And it very uncomfortable.

I expected this flavour of using:

const user = await allSettled(getUserFx, {scope, params: { userId: 31 }});

but user will be undefined in this case...

For contrast calling const user = await getUserFx({ userId: 31 }; will return user data.

Maybe in Effector exists another methods for call effects on the backend with specifying certain scope where I could get results of execution. I couldn't find.

@bacher bacher added the question Further information is requested label Sep 21, 2020
@zerobias
Copy link
Member

This imply that allSettled might fail (because if you get results, you should get errors too), if this is an expected behavior, then we could implement that idea

@zerobias
Copy link
Member

Oh, looks like errors support is a breaking change, as there are projects, which expect that allSettled is never throw an error
But we can return value with type {status: 'done', value: Done} | {status: 'fail', value: Fail}

@zerobias zerobias added enhancement New feature or request core effector package labels Sep 21, 2020
@bacher
Copy link
Author

bacher commented Sep 22, 2020

I've thought again, and seems allSettled semantic really shouldn't return results/throw errors.
And maybe we need another method/helper for calling effects for specific scope directly.

Maybe like:

const user = await getUserFx.withScope(scope)({ userId: 31 });
// or
const user = await scope.exec(getUserFx, { userId: 31 });

But in this case we're losing ability "Call provided event in scope and wait for finishing all the triggered effect." from allSettled although on backend we should wait for all asyncs.

@bacher
Copy link
Author

bacher commented Sep 22, 2020

But we can return value with type {status: 'done', value: Done} | {status: 'fail', value: Fail}

Actually I think your variant is very good. Semantic is really good, no errors, but details.

@zerobias
Copy link
Member

Okay, then we'll implement that in next minor release

@zerobias
Copy link
Member

zerobias commented Oct 2, 2020

Updates: this feature was implemented in runtime and in typings, the last question is what this method should return when used with events? Is it ok to return void? Or if we decided to return similar object, which status should be used? Can we add return object later?

After thinking about this for some time, I came to conclusion that calling allSettled with event should not return status object, at least it not useful in discussed case, but it is ok to introduce it later

@zerobias
Copy link
Member

zerobias commented Oct 5, 2020

Implemented in 21.4.0

@zerobias zerobias closed this as completed Oct 5, 2020
@bacher
Copy link
Author

bacher commented Oct 5, 2020

@zerobias Thank you very much! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core effector package enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants