fbjs
fbjs copied to clipboard
facebook
Fix Promise.finally Non-standard behavior
Standard behavior: Promise.prototype.finally()
Related issue: react-native#17972, react-native#19490, fbjs#132
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 [email protected]. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@hramos - this was legit for RN. See https://github.com/facebook/fbjs/pull/95. If it's not longer needed, can you confirm? We can ship a new fbjs 0.8.x release so RN doesn't have to update to the newer releases.
@zpao, React Native still uses Promise.native.js. Thoughts on how to migrate to the specification-compliant implementation?
@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 finally() callsites and get an idea of impact.
Happy to run the release here when you're ready. RN is locked at a version so can happen on separate schedules.
@zpao is there an ETA for this to land? Agree with you that logistically it's safe since RN is locked at a specific version. Colleague of mine lost a day and a half to this in RN so the struggle in the community is still real.
cc/ @hramos
Now that JSC on Android is updated, maybe we can continue with this PR? @yungsters
