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.

Sign up for GitHub

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

Make usePageContext return the correct path no matter where its called #21433

Merged
merged 8 commits into from
Apr 26, 2024
Merged

Make usePageContext return the correct path no matter where its called #21433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a89409
use recoil instead to break free the real current path and specific path
salazarm Apr 25, 2024
7d1cc93
use recoil instead to break free the real current path and specific path
salazarm Apr 25, 2024
2ec554b
.
salazarm Apr 25, 2024
2fcd67a
useLayoutEffect
salazarm Apr 25, 2024
fa84fb1
add recoil roots to tests
salazarm Apr 25, 2024
4ff668b
rm console.log
salazarm Apr 25, 2024
14cfd1c
add missing track page view call
salazarm Apr 25, 2024
c39367f
fix test a different way to preserve old behavior
salazarm Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions js_modules/dagster-ui/packages/app-oss/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"react-router": "^5.2.1",
"react-router-dom": "^5.3.0",
"react-router-dom-v5-compat": "^6.3.0",
"recoil": "^0.7.7",
"styled-components": "^5.3.3",
"uuid": "^9.0.0",
"validator": "^13.7.0",
Expand Down
35 changes: 19 additions & 16 deletions js_modules/dagster-ui/packages/app-oss/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {logLink, timeStartLink} from '@dagster-io/ui-core/app/apolloLinks';
import {DeploymentStatusType} from '@dagster-io/ui-core/instance/DeploymentStatusProvider';
import {LiveDataPollRateContext} from '@dagster-io/ui-core/live-data-provider/LiveDataProvider';
import {Suspense} from 'react';
import {RecoilRoot} from 'recoil';

import {InjectedComponents} from './InjectedComponents';
import {CommunityNux} from './NUX/CommunityNux';
Expand Down Expand Up @@ -40,21 +41,23 @@ const appCache = createAppCache();
// eslint-disable-next-line import/no-default-export
export default function AppPage() {
return (
<InjectedComponents>
<LiveDataPollRateContext.Provider value={liveDataPollRate ?? 2000}>
<AppProvider appCache={appCache} config={config}>
<AppTopNav allowGlobalReload>
<HelpMenu showContactSales={false} />
<UserSettingsButton />
</AppTopNav>
<App>
<ContentRoot />
<Suspense>
<CommunityNux />
</Suspense>
</App>
</AppProvider>
</LiveDataPollRateContext.Provider>
</InjectedComponents>
<RecoilRoot>
<InjectedComponents>
<LiveDataPollRateContext.Provider value={liveDataPollRate ?? 2000}>
<AppProvider appCache={appCache} config={config}>
<AppTopNav allowGlobalReload>
<HelpMenu showContactSales={false} />
<UserSettingsButton />
</AppTopNav>
<App>
<ContentRoot />
<Suspense>
<CommunityNux />
</Suspense>
</App>
</AppProvider>
</LiveDataPollRateContext.Provider>
</InjectedComponents>
</RecoilRoot>
);
}
1 change: 1 addition & 0 deletions js_modules/dagster-ui/packages/ui-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
"react-is": "^18.2.0",
"react-markdown": "^8.0.6",
"react-virtualized": "^9.22.3",
"recoil": "^0.7.7",
"rehype-highlight": "^6.0.0",
"rehype-sanitize": "^5.0.1",
"remark": "^14.0.2",
Expand Down
63 changes: 35 additions & 28 deletions js_modules/dagster-ui/packages/ui-core/src/app/__tests__/analytics.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {render, screen, waitFor} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';
import {MemoryRouter, Route, Switch} from 'react-router-dom';
import {RecoilRoot} from 'recoil';

import {AnalyticsContext, GenericAnalytics, useTrackEvent, useTrackPageView} from '../analytics';

Expand Down Expand Up @@ -32,15 +33,17 @@ describe('Analytics', () => {
const mockAnalytics = createMockAnalytics();

render(
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
</Test>
</MemoryRouter>,
<RecoilRoot>
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
</Test>
</MemoryRouter>
</RecoilRoot>,
);

jest.advanceTimersByTime(400);
Expand Down Expand Up @@ -68,15 +71,17 @@ describe('Analytics', () => {
const mockAnalytics = createMockAnalytics();

render(
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
</Test>
</MemoryRouter>,
<RecoilRoot>
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
</Test>
</MemoryRouter>
</RecoilRoot>,
);

const button = screen.getByRole('button');
Expand All @@ -103,17 +108,19 @@ describe('Analytics', () => {
const overrideAnalytics = createMockAnalytics();

render(
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Test mockAnalytics={overrideAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
<RecoilRoot>
<MemoryRouter initialEntries={['/foo/hello']}>
<Test mockAnalytics={mockAnalytics}>
<Test mockAnalytics={overrideAnalytics}>
<Switch>
<Route path="/foo/:bar?">
<Page />
</Route>
</Switch>
</Test>
</Test>
</Test>
</MemoryRouter>,
</MemoryRouter>
</RecoilRoot>,
);

jest.advanceTimersByTime(400);
Expand Down
32 changes: 21 additions & 11 deletions js_modules/dagster-ui/packages/ui-core/src/app/analytics.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import {createContext, useCallback, useContext, useEffect, useMemo} from 'react';
import {createContext, useCallback, useContext, useLayoutEffect} from 'react';
import {useLocation, useRouteMatch} from 'react-router-dom';
import {atom, useRecoilValue, useSetRecoilState} from 'recoil';

export const currentPageAtom = atom<{path: string; specificPath: string}>({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite sure I follow how Recoil avoids another provider / ordering problem, this PR adds a RecoilProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Recoil provider is at the root, that's pretty much the extent of ordering that we need to think about.

key: 'currentPageAtom',
default: {path: '/', specificPath: '/'},
});

export interface GenericAnalytics {
group?: (groupId: string, traits?: Record<string, any>) => void;
Expand All @@ -13,10 +19,7 @@ export const AnalyticsContext = createContext<GenericAnalytics>(undefined!);
const PAGEVIEW_DELAY = 300;

export const usePageContext = () => {
const match = useRouteMatch();
const {pathname: specificPath} = useLocation();
const {path} = match;
return useMemo(() => ({path, specificPath}), [path, specificPath]);
return useRecoilValue(currentPageAtom);
};

const useAnalytics = () => {
Expand Down Expand Up @@ -52,28 +55,35 @@ export const dummyAnalytics = () => ({

export const useTrackPageView = () => {
const analytics = useAnalytics();
const {path, specificPath} = usePageContext();
const match = useRouteMatch();
const {pathname: specificPath} = useLocation();
const {path} = match;

const setCurrentPage = useSetRecoilState(currentPageAtom);

useEffect(() => {
useLayoutEffect(() => {
// Wait briefly to allow redirects.
const timer = setTimeout(() => {
analytics.page(path, specificPath);
}, PAGEVIEW_DELAY);
setCurrentPage({path, specificPath});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


return () => {
clearTimeout(timer);
};
}, [analytics, path, specificPath]);
}, [analytics, path, setCurrentPage, specificPath]);
};

export const useTrackEvent = () => {
const analytics = useAnalytics();
const pathValues = usePageContext();
const match = useRouteMatch();
const {pathname: specificPath} = useLocation();
const {path} = match;

return useCallback(
(eventName: string, properties?: Record<string, any>) => {
analytics.track(eventName, {...properties, ...pathValues});
analytics.track(eventName, {...properties, path, specificPath});
},
[analytics, pathValues],
[analytics, path, specificPath],
);
};
61 changes: 34 additions & 27 deletions js_modules/dagster-ui/packages/ui-core/src/instance/backfill/__tests__/BackfillPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {MockedProvider} from '@apollo/client/testing';
import {getAllByText, getByText, render, screen, waitFor} from '@testing-library/react';
import {MemoryRouter, Route} from 'react-router-dom';
import {RecoilRoot} from 'recoil';

import {AnalyticsContext} from '../../../app/analytics';
import {
Expand Down Expand Up @@ -82,15 +83,17 @@ const mocks = [
describe('BackfillPage', () => {
it('renders the loading state', async () => {
render(
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={mocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>,
<RecoilRoot>
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={mocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>
</RecoilRoot>,
);

expect(await screen.findByTestId('page-loading-indicator')).toBeInTheDocument();
Expand All @@ -116,31 +119,35 @@ describe('BackfillPage', () => {
];

const {getByText} = render(
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={errorMocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>,
<RecoilRoot>
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={errorMocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>
</RecoilRoot>,
);

await waitFor(() => expect(getByText('An error occurred')).toBeVisible());
});

it('renders the loaded state', async () => {
render(
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={mocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>,
<RecoilRoot>
<AnalyticsContext.Provider value={{page: () => {}} as any}>
<MemoryRouter initialEntries={[`/backfills/${mockBackfillId}`]}>
<Route path="/backfills/:backfillId">
<MockedProvider mocks={mocks}>
<BackfillPage />
</MockedProvider>
</Route>
</MemoryRouter>
</AnalyticsContext.Provider>
</RecoilRoot>,
);

// Check if the loaded content is displayed
Expand Down
43 changes: 23 additions & 20 deletions js_modules/dagster-ui/packages/ui-core/src/testing/TestProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {loader} from 'graphql.macro';
import * as React from 'react';
import {MemoryRouter, MemoryRouterProps} from 'react-router-dom';
import {RecoilRoot} from 'recoil';

import {ApolloTestProps, ApolloTestProvider} from './ApolloTestProvider';
import {AppContext, AppContextValue} from '../app/AppContext';
Expand Down Expand Up @@ -73,25 +74,27 @@ export const TestProvider = (props: Props) => {
);

return (
<AppContext.Provider value={{...testValue, ...appContextProps}}>
<WebSocketContext.Provider value={websocketValue}>
<PermissionsContext.Provider
value={{
unscopedPermissions: extractPermissions(permissions),
locationPermissions: {}, // Allow all permissions to fall back
loading: false,
rawUnscopedData: [],
}}
>
<AnalyticsContext.Provider value={analytics}>
<MemoryRouter {...routerProps}>
<ApolloTestProvider {...apolloProps} typeDefs={typeDefs as any}>
<WorkspaceProvider>{props.children}</WorkspaceProvider>
</ApolloTestProvider>
</MemoryRouter>
</AnalyticsContext.Provider>
</PermissionsContext.Provider>
</WebSocketContext.Provider>
</AppContext.Provider>
<RecoilRoot>
<AppContext.Provider value={{...testValue, ...appContextProps}}>
<WebSocketContext.Provider value={websocketValue}>
<PermissionsContext.Provider
value={{
unscopedPermissions: extractPermissions(permissions),
locationPermissions: {}, // Allow all permissions to fall back
loading: false,
rawUnscopedData: [],
}}
>
<AnalyticsContext.Provider value={analytics}>
<MemoryRouter {...routerProps}>
<ApolloTestProvider {...apolloProps} typeDefs={typeDefs as any}>
<WorkspaceProvider>{props.children}</WorkspaceProvider>
</ApolloTestProvider>
</MemoryRouter>
</AnalyticsContext.Provider>
</PermissionsContext.Provider>
</WebSocketContext.Provider>
</AppContext.Provider>
</RecoilRoot>
);
};
Loading