Make usePageContext return the correct path no matter where its called
#21433
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit c39367f. |
salazarm
force-pushed
the
salazarm/remove-timeout
branch
from
ff073fa to
3a89409
Compare
salazarm
changed the title
analytics.pageuseTrackPageView
salazarm
changed the title
useTrackPageViewusePageContext to work anywhere and return the correct path
salazarm
changed the title
usePageContext to work anywhere and return the correct pathusePageContext return the correct path no matter where its called
| import {useLocation, useRouteMatch} from 'react-router-dom'; | ||
| import {atom, useRecoilValue, useSetRecoilState} from 'recoil'; | ||
|
|
||
| export const currentPageAtom = atom<{path: string; specificPath: string}>({ |
There was a problem hiding this comment.
I used a recoil atom here to avoid having yet another provider and having to order them properly (which in some cases necessitates another provider yay). Cloud already uses Recoil
There was a problem hiding this comment.
I'm not quite sure I follow how Recoil avoids another provider / ordering problem, this PR adds a RecoilProvider?
There was a problem hiding this comment.
The Recoil provider is at the root, that's pretty much the extent of ordering that we need to think about.
| import {useLocation, useRouteMatch} from 'react-router-dom'; | ||
| import {atom, useRecoilValue, useSetRecoilState} from 'recoil'; | ||
|
|
||
| export const currentPageAtom = atom<{path: string; specificPath: string}>({ |
There was a problem hiding this comment.
I'm not quite sure I follow how Recoil avoids another provider / ordering problem, this PR adds a RecoilProvider?
| // Wait briefly to allow redirects. | ||
| const timer = setTimeout(() => { | ||
| analytics.page(path, specificPath); | ||
| }, PAGEVIEW_DELAY); | ||
| setCurrentPage({path, specificPath}); |
There was a problem hiding this comment.
This all looks good and I think I'm in favor of the formal React solution. That said, an alternative two line version of this PR would be to set a hidden window.__routerPath = path here, and then have usePageContext return {path: window.__routerPath, specificPath: window.location.path}.
There was a problem hiding this comment.
This wouldn't be equivalent because changing __routerPath wouldn't let anyone relying on the path know that it changed and that's key for page load logging.
…led (dagster-io#21433) ## Summary & Motivation To log page load we need to inform datadog when a new view starts. To do this we currently rely on `usePageContext` to change when the path changes. Currently it doesn't do that because `usePageContext` relies on `useRouteMatch` from react-router-dom which finds the path from the closest parent `Route`. The issue is that we're calling `usePageContext` from a provider that is very high up in the tree, so `useRouteMatch` can't access the context of the `<Route>` currently being rendered. Instead of using `useRouteMatch` lets store the path in global state when `useTrackPageView` is called and have `usePageContext` rely on that global state. Cloud requires this change to dedupe recoil: https://github.com/dagster-io/internal/pull/9471 ## How I Tested These Changes 👀 , play around with the app make sure nothing is broken
Summary & Motivation
To log page load we need to inform datadog when a new view starts. To do this we currently rely on
usePageContextto change when the path changes. Currently it doesn't do that becauseusePageContextrelies onuseRouteMatchfrom react-router-dom which finds the path from the closest parentRoute. The issue is that we're callingusePageContextfrom a provider that is very high up in the tree, souseRouteMatchcan't access the context of the<Route>currently being rendered. Instead of usinguseRouteMatchlets store the path in global state whenuseTrackPageViewis called and haveusePageContextrely on that global state.Cloud requires this change to dedupe recoil: https://github.com/dagster-io/internal/pull/9471
How I Tested These Changes
👀 , play around with the app make sure nothing is broken