Skip to content

fix: Warn when promise rejections won't be caught#1886

Merged
marandaneto merged 24 commits intomasterfrom
promise
Dec 1, 2021
Merged

fix: Warn when promise rejections won't be caught#1886
marandaneto merged 24 commits intomasterfrom
promise

Conversation

@jennmueng
Copy link
Contributor

@jennmueng jennmueng commented Nov 12, 2021

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This fix will appear to not work in the original sample app in this repo as our SDK will use the promise library from the root folder instead of the one inside the sample folder that React Native uses. Thus the end to end tests have been updated to use a packaged version of the SDK via yalc. This way the promise rejection handler will be tested.

e2e test workflow will run the script sample/scripts/prepareConfigsForTesting.sh which will swap out the babel config and the metro config to the testing one which will use the packaged SDK instead.

Extra

  • Removes Flipper from Android sample
  • Also the behavior of the promise rejection handler will follow the behavior of the default react native handler rather than just calling. a console.warn like before.

💡 Motivation and Context

Fixes #1077

💚 How did you test it?

End to end tests will now package the SDK and test the unhandled promise instrumentation.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Merge docs PR getsentry/sentry-docs#4392

@jennmueng
Copy link
Contributor Author

For some reason the android emulator for the e2e test isn't working, says app never started. Works locally... going to keep debugging this

@jennmueng jennmueng closed this Nov 15, 2021
@jennmueng jennmueng reopened this Nov 15, 2021
@jennmueng
Copy link
Contributor Author

Accidentally hit close

@jennmueng jennmueng marked this pull request as ready for review November 16, 2021 03:33
@jennmueng jennmueng requested a review from romtsn as a code owner November 16, 2021 03:33
@marandaneto
Copy link
Contributor

@AbhiPrasad would you mind reviewing this one too? that's a field I don't master, thanks.

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Copy link
Contributor

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, sorry for the late review. I'll make sure the ✅ comes back way faster :)))

run: sh ./scripts/prepareConfigsForTesting.sh
- name: Install SDK in sample
working-directory: ./sample
run: yalc add @sentry/react-native
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think its unavoidable now, but lets please split up the CI changes from the implementation so that things are easier to revert/git blame in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, but splitting up would break the e2e since the CI changes are needed, let's keep this in mind for the next PR but I'd rather merge it for now since it's an important fix.

@jennmueng
Copy link
Contributor Author

Thanks @AbhiPrasad! Just got back from thanksgiving break and addressed your review. Next time will make separate PRs for CI changes.

@marandaneto
Copy link
Contributor

thanks for reviewing @AbhiPrasad

@marandaneto marandaneto merged commit f1ff526 into master Dec 1, 2021
@marandaneto marandaneto deleted the promise branch December 1, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled promises are not logged at all (iOS and Android)

4 participants