Fix Promise.finally Non-standard behavior #293
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
How come this isn't merged yet. |
|
@zpao, React Native still uses |
| throw reason; | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
FWIWI: We can replace this whole definition with just a call to require('promise/setimmediate/finally'). https://github.com/then/promise/blob/7.3.1/src/finally.js
What you have is virtually identical, just reducing the amount of code we need to have written ourselves.
|
@yungsters Does RN's runtime not have a native Promise impl yet? Feels like we should do that, then swap this module out for something that just returns the existing global. |
|
React Native's version of JSC on Android is super old. On iOS, we delegate to iOS. 👎 |
|
Ok, then this (or my recommendation) are effectively the same as the spec (minus some nuance). There's https://github.com/es-shims/Promise.prototype.finally/blob/master/implementation.js but that's looking pretty overkill, especially since we might not even have a native Promise to begin with. There are only a couple small differences between the old impl and this. Namely resolution/reject value. Alternatively, you could ship the exact Promise polyfill you want in RN, and then the module here could just check for the global first. May lead to some duplicate packaging though since it'd be a runtime check. As for rolling it out… obviously it could lead to behavior changes. I'd probably do an internal audit of Happy to run the release here when you're ready. RN is locked at a version so can happen on separate schedules. |
|
Now that JSC on Android is updated, maybe we can continue with this PR? @yungsters |
Summary: See facebook/fbjs#293 for an explanation. Reviewed By: yungsters Differential Revision: D14658680 fbshipit-source-id: aad8808b8514817314bf67b9a43b01d52e850e36
Standard behavior: Promise.prototype.finally()
Related issue: react-native#17972, react-native#19490, fbjs#132