Make InteractionManager.runAfterInteractions() return a Promise #3788
Conversation
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
cc @joesavona |
|
(Hi! 👋) |
|
This is pretty useful, we do something similar in the Exponent fork of react-timer-mixin, called react-native-timer-mixin: expo/react-native-timer-mixin@8064622 This allows us to do:
And it protects us from the component potentially being unmounted. One potential issue with using the approach from this PR is that it is possible for the component to be unmounted between when the function yields and regains control, so you'd have to always wrap it in try / catch. But it's still useful outside of components. 👍 |
|
Are those CI failures for real, btw? Seems unlikely that this affected layout... |
| _queue.push(callback); | ||
| } | ||
| _queue.push(resolve); | ||
| }); |
There was a problem hiding this comment.
Returning a promise seems fine, but the callback has to be executed after interactions, not when the promise is created. How about:
runAfterInteractions(callback: ?Function): Promise {
return new Promise(resolve => {
scheduleUpdate();
_queue.push(() => {
callback && callback();
resolve();
});
});
}Also, both of these implementations will cause the promise to never be resolved if executing callback throws an error. Is that intended?
There was a problem hiding this comment.
The callback is pushed onto the queue i.e. invoked when the interaction completes. That's my reading of @philikon's code at least. The InteractionManager wraps each callback in an ErrorUtils guard so even if the callback fails the InteractionManager will run the promise's resolve function.
There was a problem hiding this comment.
Yeah what @ide says. I didn't change the semantics around how callback gets executed. If you read the red portions of the diff, you'll see that it currently gets pushed onto the queue just like it does after my change.
There was a problem hiding this comment.
Yup, I read this too fast.
👍
|
Pingsies? |
|
Meow. |
|
@facebook-github-bot import |
|
Sorry about the wait! |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1637419186517901/int_phab to review. |
ghost
closed this
in
Summary: In accordance with the unwritten rule that any API that takes a callback should also return a promise, I've changed `InteractionManager.runAfterInteractions()` to do just that.
```js
InteractionManager.runAfterInteractions(() => {
...
});
```
can become
```js
InteractionManager.runAfterInteractions().then(() => {
...
});
```
(but doesn't have to). Most importantly, though, this change enables code like
```js
doSomeUIStuff();
await InteractionManager.runAfterInteractions();
doSomeNonUIStuff();
```
which is nice.
Note: Because returning a `Promise` means the callback argument is now optional, the behaviour of the API is slightly changed, though not in a backwards-incompatible way (unless a consumer is in some way relying on the exception, but that would be insane).
Closes facebook#3788
Reviewed By: vjeux
Differential Revision: D2634693
Pulled By: josephsavona
fb-gh-sync-id: 7315120963be23cf69d777e940b2750d32ae47a8
Summary: In accordance with the unwritten rule that any API that takes a callback should also return a promise, I've changed `InteractionManager.runAfterInteractions()` to do just that.
```js
InteractionManager.runAfterInteractions(() => {
...
});
```
can become
```js
InteractionManager.runAfterInteractions().then(() => {
...
});
```
(but doesn't have to). Most importantly, though, this change enables code like
```js
doSomeUIStuff();
await InteractionManager.runAfterInteractions();
doSomeNonUIStuff();
```
which is nice.
Note: Because returning a `Promise` means the callback argument is now optional, the behaviour of the API is slightly changed, though not in a backwards-incompatible way (unless a consumer is in some way relying on the exception, but that would be insane).
Closes facebook#3788
Reviewed By: vjeux
Differential Revision: D2634693
Pulled By: josephsavona
fb-gh-sync-id: 7315120963be23cf69d777e940b2750d32ae47a8
Summary: In accordance with the unwritten rule that any API that takes a callback should also return a promise, I've changed `InteractionManager.runAfterInteractions()` to do just that.
```js
InteractionManager.runAfterInteractions(() => {
...
});
```
can become
```js
InteractionManager.runAfterInteractions().then(() => {
...
});
```
(but doesn't have to). Most importantly, though, this change enables code like
```js
doSomeUIStuff();
await InteractionManager.runAfterInteractions();
doSomeNonUIStuff();
```
which is nice.
Note: Because returning a `Promise` means the callback argument is now optional, the behaviour of the API is slightly changed, though not in a backwards-incompatible way (unless a consumer is in some way relying on the exception, but that would be insane).
Closes facebook#3788
Reviewed By: vjeux
Differential Revision: D2634693
Pulled By: josephsavona
fb-gh-sync-id: 7315120963be23cf69d777e940b2750d32ae47a8
In accordance with the unwritten rule that any API that takes a callback should also return a promise, I've changed
InteractionManager.runAfterInteractions()to do just that.can become
(but doesn't have to). Most importantly, though, this change enables code like
which is nice.
Note: Because returning a
Promisemeans the callback argument is now optional, the behaviour of the API is slightly changed, though not in a backwards-incompatible way (unless a consumer is in some way relying on the exception, but that would be insane).