fbjs icon indicating copy to clipboard operation
fbjs copied to clipboard

Bump "promise" to ^8.0.3 to match React Native

Open bengourley opened this issue 3 years ago • 13 comments

tl;dr out of sync dependencies on "promise" in React Native vs. fbjs mean that two different versions get installed on a project, and unhandled promise rejections do not get reported in Bugsnag on RN 0.63+ and Expo SDK39+


React Native 0.63 (and by extension Expo SDK 39-40) bumped its dependency on "promise" to be ^8.0.3 ¹

This repo depends on promise@^7.1.1 which has no valid overlapping versions. This module "fbjs" is depended on by "metro", which in turn is depended on by RN/Expo projects.

This means that on a standard RN/Expo project, two versions of "promise" get installed, making it impossible to obtain a reference to the same version that React Native is using², and thus impossible to detect unhandled promise rejections – and for example report them to Bugsnag.

¹ https://github.com/facebook/react-native/commit/b23efc526446cad53adc3de7465fd8be7309d84e ² https://github.com/bugsnag/bugsnag-js/blob/eb6e05f350f5acf496c9e3dfd341c9061c400c1f/packages/plugin-react-native-unhandled-rejection/rejection-handler.js#L5

bengourley avatar Jan 05 '21 15:01 bengourley

Hi @bengourley!

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 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 to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot avatar Jan 05 '21 15:01 facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot avatar Jan 05 '21 16:01 facebook-github-bot

could this PR be merged ?

jer-sen avatar Jun 16 '21 15:06 jer-sen

Any hope of getting this merged? We just ran into this in our project...

tkieft avatar Jul 08 '21 18:07 tkieft

React Native stopped using fbjs a year ago (https://github.com/facebook/react-native/commit/54e19a6b7f217ffc0611e660f2a6b1a8ad14775b), as did metro (https://github.com/facebook/metro/commit/203fb31ec028f9f147bd0a1b689b587a396d9b37). I know RN has done a release or 2 since then so I'd recommend upgrading RN as a better approach. Merging and shipping a new current fbjs release doesn't help, so this needs to be applied to a 3 year old release of fbjs, which may have other consequences.

zpao avatar Jul 08 '21 19:07 zpao

Great, so it looks like the fix is to update to a version of RN >= 0.64.0 (which transitively includes metro >= 0.64.0). Thanks!

tkieft avatar Jul 08 '21 19:07 tkieft

@zpao react-native-web still relies on fbjs which cause me trouble. Why can't you merge this PR?

jer-sen avatar Oct 19 '21 21:10 jer-sen

@jer-sen react-native-dom is using fbjs but it's usage is limited and doesn't import anything that uses the promise module. What problems are you seeing?

I'm not against shipping this just the issue people were having when reported is not relevant anymore.

zpao avatar Oct 20 '21 03:10 zpao

@zpao I use sentry-expo for bug tracking (as a LOT of people using Expo since its the recommanded and only supported bug tracker for managed workflow: https://docs.expo.dev/guides/using-sentry/). It needs to hook on Promise global object to report failed or unhandled promised. To do so it relies on promise module but it needs to use the same version than the one that hooked on Promise global object, otherwise Sentry do not received events. React Native depends on promise: "^8.0.3" but 2 other modules depend inderectly on an other version of promise through fbjs: react-native-web#fbjs#promise & expo#fbemitter#fbjs#promise. If fbjs dependency on promise were bumped to ^8.0.3 there would be only one version in my project and everything would work fine (as it does with this addition to my package.json: "resolutions": { "react-native-web/**/promise": "^8.0.3", "expo/**/promise": "^8.0.3" },).

So, could you merge this PR ? Also expo will have to stop relying on the archived project fbemitter which depends on an old version of fbjs.

jer-sen avatar Oct 20 '21 06:10 jer-sen

Best I can tell, promise is never actually required in any paths used by fbemitter nor react-native-web. Just because the old version is a dependency somewhere doesn't mean it's actually used. Do you have a stack trace or something indicating it is somehow a problem.

zpao avatar Oct 21 '21 18:10 zpao

You are right. In fact, what happens is that sentry-expo requires promise module to register an event. Cf https://github.com/getsentry/sentry-react-native/blob/e3d11a1b657f1864af41414dced73394f03e9b23/src/js/integrations/reactnativeerrorhandlers.ts#L83

But the version resolved is not the one from react-native at ^8.0.3, it's an other hoisted for react-native-web#fbjs#promise and expo#fbemitter#fbjs#promise at 7.3.1. So the event is not registered on the global Promise object installed by react-native.

I see to possible fixes:

  • bumping promise version of fbjs => all modules will then use the same version that will be hoisted
  • adding a dependency to my project at the same version than react-native => react-native will use this hoisted version which will be the one resolved by sentry-expo's require

The second one seems cleaner to me. I will suggest Sentry to update there installation doc and add a runtime check.

Sorry for the mistake in my previous comment and many thanks for invastigating my issue.

jer-sen avatar Oct 22 '21 12:10 jer-sen

No worries, module dependency resolution and po{l,n}yfills are a huge mess. It's a totally legit issue much of the time. Like I said, I'm not against updating here at all. I do hesitate to do major bumps of dependencies like this without also doing a major version bump of fbjs itself. That said, the diff (https://github.com/then/promise/compare/7.1.1...8.0.3) is essentially just adding typedefs for flow and typescript and updating an error message, so it's probably fine. 8.1.0 has some new support but for this case, wouldn't jump to there in a minor version of fbjs.

zpao avatar Oct 22 '21 21:10 zpao

Thoughts on landing this? Just spent a couple of hours debugging a maximum call stack error that in the end was resolved by removing our core-js polyfill of Promise.allSettled and adding a resolutions for promise to force v8 (and disabling Sentry's polyfilling). As with above, v7 from fbjs was the one hoisted and the one @sentry/react-native ended up pulling in.

https://github.com/getsentry/sentry-react-native/pull/1984

SimenB avatar Jul 12 '23 09:07 SimenB