-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(ui): decouple scraps/link from sentry specific code #102644
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
Conversation
| /** | ||
| * A context-aware version of Link (from react-router) that falls | ||
| * back to <a> if there is no router present | ||
| */ | ||
| export const Link = styled(({disabled, to, ...props}: LinkProps) => { | ||
| const location = useLocation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evanpurkhiser do you maybe know why we have this “fall back to <a> if there is no router present. When would that trigger? Does useLocation ever return null / undefined?
I would honestly love to just remove that if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this actually dies at runtime so why do we have it 😅 ?
router.js:422 Uncaught Error: useLocation() may be used only in the context of a <Router> component.
at invariant (router.js:422:15)
at useLocation (index.js:204:104)
at useLocation (useLocation.tsx:25:90)
at SentryLinkBehaviorProvider (link.tsx:25:91)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably pretty legacy. This might have been needed at one point for one of these things
sentry/static/app/bootstrap/processInitQueue.tsx
Lines 22 to 37 in 4ffd2a0
| const COMPONENT_MAP = { | |
| [SentryInitRenderReactComponent.INDICATORS]: () => | |
| import(/* webpackChunkName: "Indicators" */ 'sentry/components/indicators'), | |
| [SentryInitRenderReactComponent.SYSTEM_ALERTS]: () => | |
| import(/* webpackChunkName: "SystemAlerts" */ 'sentry/views/app/systemAlerts'), | |
| [SentryInitRenderReactComponent.SETUP_WIZARD]: () => | |
| import(/* webpackChunkName: "SetupWizard" */ 'sentry/views/setupWizard'), | |
| [SentryInitRenderReactComponent.WEB_AUTHN_ASSSERT]: () => | |
| import( | |
| /* webpackChunkName: "WebAuthnAssert" */ 'sentry/components/webAuthn/webAuthnAssert' | |
| ), | |
| [SentryInitRenderReactComponent.SU_STAFF_ACCESS_FORM]: () => | |
| import( | |
| /* webpackChunkName: "SuperuserStaffAccessForm" */ 'sentry/components/superuserStaffAccessForm' | |
| ), | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt we need this yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I think this is what we want then: f1625c5
the design-system uses ’a’ per default unless specified by the context, which both the sentry app and our tests are doing.
so that we can call useLocation inside
JonasBa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean, I like it 🙏
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102644 +/- ##
===========================================
- Coverage 80.93% 80.92% -0.01%
===========================================
Files 8929 8906 -23
Lines 391245 390801 -444
Branches 24866 24849 -17
===========================================
- Hits 316649 316262 -387
+ Misses 74228 74151 -77
- Partials 368 388 +20 |
…inkBehaviorContext set both the sentry app and tests set that, so this is a defensive fallback
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
|
PR reverted: cdec585 |
)" This reverts commit 812ea00. Co-authored-by: JonasBa <9317857+JonasBa@users.noreply.github.com>
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
)" This reverts commit 812ea00. Co-authored-by: JonasBa <9317857+JonasBa@users.noreply.github.com>
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
)" This reverts commit 812ea00. Co-authored-by: JonasBa <9317857+JonasBa@users.noreply.github.com>
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
)" This reverts commit 812ea00. Co-authored-by: JonasBa <9317857+JonasBa@users.noreply.github.com>
This is a continuation of - #102644 which got reverted because it broke `_admin`. This version has the following changes: - We now add a `ScrapsProvider` around the root routes of `_admin`: 0f0615a - We now default to a `RouterLink` even if no Provider is found: 803054e - Falling back to an `a` tag is a bad idea because it will render `<a to=“...”>` which won’t work (this was the `_admin` issue). It turns out, this is actually how `disabled` is implemented for link, as will create links that can’t be clicked 🥲. I don’t think we need to decouple scraps from react-router right now; previously, we called `useLocation`, which would fail at runtime if there is no router provider, so we can’t use our `Link` component outside of a router context anyway. So now, we have a `ScrapsProvider` around all routes (I hope there aren’t any more places) and for cases where we have no Provider, we should still render a link from react-router.
This PR introduces a
LinkBehaviorContextthat allows us to decouple sentry specific code from our design-systemscraps.In this first iteration, the only sentry specific things were:
locationDescriptorToTouseLocationto fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have nouseLocation?I do plan to add more sentry specific link behavior, like intent prefetching, but this de-coupling is pre-requisite for that.