Skip to content

Conversation

@bghgary
Copy link
Contributor

@bghgary bghgary commented Oct 3, 2020

Better fix for #60. We're asking React Native folks to see if they can confirm our theory.

@bghgary bghgary requested a review from ryantrem October 3, 2020 00:41
// Without a call to one of these asynchronous functions, the promise continuation is not
// triggered. This creates a no-op setTimeout function that is called in the Babylon Native
// JsRuntime dispatch function to poke the promise continuation execution.
inline std::shared_ptr<facebook::jsi::Function> GetSetTimeout(facebook::jsi::Runtime& rt)
Copy link
Member

Choose a reason for hiding this comment

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

Since we are never going to actually use this as a regular setTimeout call (e.g. we will always pass in a zero duration and an empty callback), I'd be inclined to make the setTimeout part an implementation detail of what we're actually trying to achieve here - getting pending promise continuations to execute. Given that, maybe this should be more like:
inline std::function<void()> GetPendingPromiseContinuationExecutor(facebook::jsi::Runtime& rt)
I think this would help clarify the purpose of this code, and also simplify the call sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this originally, but had trouble with the move semantics. I'll see if I can get it to work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did something different. See if you're okay with it now.

@bghgary bghgary marked this pull request as ready for review October 8, 2020 20:19
@bghgary bghgary merged commit 694f3b3 into BabylonJS:master Oct 8, 2020
@bghgary bghgary deleted the promise-fix branch October 8, 2020 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants