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

Drop support for handling promises returned by fromCallback #3282

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented May 10, 2022

My main rationale for this is that it's an easy mistake to do this:

fromCallback(async (sendBack) {
  const api = await getSomeApi()
  api.onFoo = (val) => sendBack({ type: 'EV', val })
})

The problem with this code is that this promise implicitly completes very quickly and thus the parent stops accepting events sent out from this callback. Here the async was used as a convenient mechanism to use await, the intention wasn't really to complete this callback actor with undefined. Note that this is not a made-up example - this has been reported a while back on Discord by Kent and the resulting situation was quite hard to debug for a user because XState was logging cryptic errors in this case.

This can easily be refactored to:

fromCallback(() => {
  (async (sendBack) {
    const api = await getSomeApi()
    api.onFoo = (val) => sendBack({ type: 'EV', val })
  })()
})

Note that supporting promises (what is being removed from here) doesn't also play that well with current types as the return value of fromCallback is Behavior<TEvent, undefined> whereas that undefined is not a strong guarantee with a returned promise.

@Andarist Andarist requested a review from davidkpiano May 10, 2022 15:53
@changeset-bot
Copy link

changeset-bot bot commented May 10, 2022

🦋 Changeset detected

Latest commit: d6311a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
xstate Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 10, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d6311a2:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented May 10, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@Andarist
Copy link
Member Author

Alternatively we could allow promises but ignore the completion value. In such a case though - what to do about errors? We probably should handle them but then out handling wouldnt be symmetric (and thus somewhat weird)

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of this; can you re-merge the v5/improved-spawn-1 PR as there are new changes?

@Andarist
Copy link
Member Author

@davidkpiano ye, gonna handle the merge tomorrow

@mattpocock would love your input on this one too

Base automatically changed from v5/improved-spawn-1 to next May 26, 2022 10:09
@Andarist Andarist force-pushed the next branch 2 times, most recently from 0c89c96 to 98a7202 Compare May 26, 2022 12:12
@Andarist Andarist force-pushed the drop-mixed-callback-async-support branch from fc6e932 to d6311a2 Compare September 13, 2023 14:13
@Andarist Andarist changed the title Drop support for handling promises returned by invokeCallback Drop support for handling promises returned by fromCallback Sep 13, 2023
@Andarist
Copy link
Member Author

This was already approved before but it got stale and I just rebased it. @davidkpiano could you take a second look at this?

@Andarist Andarist merged commit 6ff9fc2 into next Sep 13, 2023
3 checks passed
@Andarist Andarist deleted the drop-mixed-callback-async-support branch September 13, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants