Skip to content

feat: Use beforeNavigate in routing instrumentation to match behavior on JS #1313

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

Merged
merged 10 commits into from
Feb 1, 2021

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Jan 31, 2021

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

NOTE: This needs #1311 to be merged first!

Uses the beforeNavigate option that is passed to ReactNativeTracing to have the same behavior as on JS.
(This removes the deprecated feature of returning undefined in the type, although handles it just in case users are used to the old behavior.)

We will now also export a convenience type ReactNavigationTransactionContext extends TransactionContext for users who use the React Navigation instrumentations:

beforeNavigate: (context: Sentry.ReactNavigationTransactionContext) => {
  // context.data.route, context.data.previousRoute, context.tags is typed for convenience

  return context;
}

This also does not implement any context-change-wiping that was discussed with @lobsterkatie in getsentry/sentry-javascript#3192 as I believe there needs to be more discussion internally about which properties should not be changed by the user.

💡 Motivation and Context

Addresses #1312

💚 How did you test it?

Updated tests for React Navigation V4 and V5. Tested on simulators running both instrumentations.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes. NOTE: This breaks users who used shouldSendTransaction on 2.2.0-beta.0. But it was undocumented in anticipation for this change so there should not be any or at worst very few.

@jennmueng jennmueng linked an issue Jan 31, 2021 that may be closed by this pull request
@jennmueng jennmueng self-assigned this Jan 31, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2021

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against b7e434c

@jennmueng
Copy link
Member Author

Also note, tests will fail until this is rebased on top of #1311

@jennmueng jennmueng merged commit ef2f0ff into master Feb 1, 2021
@jennmueng jennmueng deleted the jenn/performance-improve branch February 1, 2021 13:26
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.

Routing instrumentation should use beforeNavigate like on JS.
2 participants