Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump "promise" to ^8.0.3 to match React Native #413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bengourley
Copy link

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.

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

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.

¹ facebook/react-native@b23efc5
² https://github.com/bugsnag/bugsnag-js/blob/eb6e05f350f5acf496c9e3dfd341c9061c400c1f/packages/plugin-react-native-unhandled-rejection/rejection-handler.js#L5
@facebook-github-bot
Copy link

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 cla@fb.com. Thanks!

@facebook-github-bot
Copy link

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

@jer-sen
Copy link

jer-sen commented Jun 16, 2021

could this PR be merged ?

@tkieft
Copy link

tkieft commented Jul 8, 2021

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

@zpao
Copy link
Member

zpao commented Jul 8, 2021

React Native stopped using fbjs a year ago (facebook/react-native@54e19a6), as did metro (facebook/metro@203fb31). 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.

@tkieft
Copy link

tkieft commented Jul 8, 2021

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!

@jer-sen
Copy link

jer-sen commented Oct 19, 2021

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

@zpao
Copy link
Member

zpao commented Oct 20, 2021

@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.

@jer-sen
Copy link

jer-sen commented Oct 20, 2021

@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.

@zpao
Copy link
Member

zpao commented Oct 21, 2021

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.

@jer-sen
Copy link

jer-sen commented Oct 22, 2021

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.

@zpao
Copy link
Member

zpao commented Oct 22, 2021

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 (then/promise@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.

@SimenB
Copy link
Contributor

SimenB commented Jul 12, 2023

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.

getsentry/sentry-react-native#1984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants