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

Migrate router and <app-view> to React #4525

Merged
merged 37 commits into from
Jan 20, 2020
Merged

Migrate router and <app-view> to React #4525

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
ee529fe
Migrate router and <app-view> to React: skeleton
kravets-levko Jan 7, 2020
94e3de1
Update layout on route change
kravets-levko Jan 7, 2020
0e85dd8
Start moving page routes from angular to react
kravets-levko Jan 8, 2020
ce64f0d
Move page routes to react except of public dashboard and visualizatio…
kravets-levko Jan 8, 2020
4058093
Move public dashboard and visualization embed routes to React
kravets-levko Jan 8, 2020
b4ff41c
Replace $route/$routeParams usages
kravets-levko Jan 8, 2020
d874669
Some cleanup
kravets-levko Jan 8, 2020
8bb6c6d
Replace AngularJS $location service with implementation based on hist…
kravets-levko Jan 10, 2020
e4350f0
Minor fix to how ApplicationView handles route change
kravets-levko Jan 10, 2020
933b50a
Explicitly use global layout for each page instead of handling relate…
kravets-levko Jan 10, 2020
d2e0f98
Error handling
kravets-levko Jan 10, 2020
824ef59
Merge branch 'master' into migrate-router-to-react
kravets-levko Jan 13, 2020
b2b504e
Remove AngularJS and related dependencies
kravets-levko Jan 13, 2020
9815996
Move Parameter factory method to a separate file
gabrieldutra Jan 13, 2020
7306d21
Fix CSS (replace custom components with classes)
kravets-levko Jan 14, 2020
b140b55
Fix: keep other url parts when updating location partially; refine code
kravets-levko Jan 14, 2020
e075d40
Merge branch 'master' into migrate-router-to-react
kravets-levko Jan 14, 2020
4f299fe
Fix tests
kravets-levko Jan 14, 2020
0336dad
Make router work in multi-org mode (respect <base> tag)
kravets-levko Jan 14, 2020
79111f2
Optimzation: don't resolve route if path didn't change
kravets-levko Jan 14, 2020
5c94b70
Fix search input in header; error handling improvement (handle more e…
kravets-levko Jan 16, 2020
ebaf1e2
Fix page keys; fix navigateTo calls (third parameter not available)
kravets-levko Jan 16, 2020
09c096c
Use relative links
kravets-levko Jan 16, 2020
5b7d56c
Router: ignore location REPLACE events, resolve only on PUSH/POP
kravets-levko Jan 16, 2020
f77f602
Fix tests
kravets-levko Jan 16, 2020
efeecb6
Remove unused jQuery reference
kravets-levko Jan 16, 2020
6e6c1df
Merge branch 'master' into migrate-router-to-react
kravets-levko Jan 16, 2020
d2e0beb
Show error from backend when creating Destination
gabrieldutra Jan 16, 2020
d4a769b
Remove route.resolve where not necessary (used constant values)
kravets-levko Jan 19, 2020
dea46de
New Query page: keep state on saving, reload when creating another ne…
kravets-levko Jan 19, 2020
dc4051a
Use currentRoute.key instead of hard-coded keys for page components
kravets-levko Jan 19, 2020
a54d917
Tidy up Router
kravets-levko Jan 19, 2020
d822d39
Tidy up location service
kravets-levko Jan 19, 2020
a92d1fa
Fix tests
kravets-levko Jan 19, 2020
2755e12
Merge branch 'master' into migrate-router-to-react
kravets-levko Jan 20, 2020
f8e7334
Don't add parameters changes to browser's history
kravets-levko Jan 20, 2020
7e3f9cf
Fix test (improved fix)
kravets-levko Jan 20, 2020
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
21 changes: 16 additions & 5 deletions client/app/components/ApplicationArea/Router.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ import PromiseRejectionError from "@/lib/promise-rejection-error";

import ErrorMessage from "./ErrorMessage";

function generateRouteKey() {
return Math.random()
.toString(32)
.substr(2);
}

export function stripBase(href) {
// Resolve provided link and '' (root) relative to document's base.
// If provided href is not related to current document (does not
Expand Down Expand Up @@ -54,7 +60,7 @@ export default function Router({ routes, onRouteChange }) {
},
});

function resolve() {
function resolve(action) {
if (!isAbandoned) {
if (errorHandlerRef.current) {
errorHandlerRef.current.reset();
Expand All @@ -73,14 +79,19 @@ export default function Router({ routes, onRouteChange }) {
}
currentPathRef.current = pathname;

// Don't reload controller if URL was replaced
if (action === "REPLACE") {
return;
}

router
.resolve({ pathname })
.then(route => {
return isAbandoned || currentPathRef.current !== pathname ? null : resolveRouteDependencies(route);
})
.then(route => {
if (route) {
setCurrentRoute(route);
setCurrentRoute({ ...route, key: generateRouteKey() });
}
})
.catch(error => {
Expand All @@ -94,9 +105,9 @@ export default function Router({ routes, onRouteChange }) {
}
}

resolve();
resolve("PUSH");

const unlisten = location.listen(resolve);
const unlisten = location.listen((unused, action) => resolve(action));

return () => {
isAbandoned = true;
Expand All @@ -116,7 +127,7 @@ export default function Router({ routes, onRouteChange }) {
<ErrorBoundary
ref={errorHandlerRef}
renderError={error => <ErrorMessage error={error} showOriginalMessage={false} />}>
{currentRoute.render(currentRoute.routeParams, currentRoute, location)}
{currentRoute.render(currentRoute)}
</ErrorBoundary>
);
}
Expand Down
10 changes: 9 additions & 1 deletion client/app/components/ApplicationArea/navigateTo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ export default function navigateTo(href, replace = false) {
window.location = href;
return;
}
location.update(url.parse(href), replace);
href = url.parse(href);
location.update(
{
path: href.pathname,
search: href.search,
hash: href.hash,
},
replace
);
}, 10);
}
7 changes: 3 additions & 4 deletions client/app/pages/admin/Jobs.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,11 @@ class Jobs extends React.Component {
export default {
path: "/admin/queries/jobs",
title: "RQ Status",
render: routeParams => (
<AuthenticatedPageWrapper key="/admin/queries/jobs">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <Jobs {...routeParams} onError={handleError} />}
{({ handleError }) => <Jobs {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "jobs" },
};
11 changes: 7 additions & 4 deletions client/app/pages/admin/OutdatedQueries.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,17 @@ const OutdatedQueriesPage = itemsList(
export default {
path: "/admin/queries/outdated",
title: "Outdated Queries",
render: (routeParams, currentRoute) => (
<AuthenticatedPageWrapper key="/admin/queries/outdated">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => (
<OutdatedQueriesPage routeParams={routeParams} currentRoute={currentRoute} onError={handleError} />
<OutdatedQueriesPage
routeParams={{ ...currentRoute.routeParams, currentPage: "outdated_queries" }}
currentRoute={currentRoute}
onError={handleError}
/>
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "outdated_queries" },
};
7 changes: 3 additions & 4 deletions client/app/pages/admin/SystemStatus.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,11 @@ class SystemStatus extends React.Component {
export default {
path: "/admin/status",
title: "System Status",
render: routeParams => (
<AuthenticatedPageWrapper key="/admin/status">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <SystemStatus {...routeParams} onError={handleError} />}
{({ handleError }) => <SystemStatus {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "system_status" },
};
21 changes: 9 additions & 12 deletions client/app/pages/alert/Alert.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,37 +258,34 @@ export default [
{
path: "/alerts/new",
title: "New Alert",
render: routeParams => (
<AuthenticatedPageWrapper key="/alerts/new">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <AlertPage {...routeParams} onError={handleError} />}
{({ handleError }) => <AlertPage {...currentRoute.routeParams} mode={MODES.NEW} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { mode: MODES.NEW },
},
{
path: "/alerts/:alertId([0-9]+)",
title: "Alert",
render: routeParams => (
<AuthenticatedPageWrapper key={`/alerts/${routeParams.alertId}`}>
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <AlertPage {...routeParams} onError={handleError} />}
{({ handleError }) => <AlertPage {...currentRoute.routeParams} mode={MODES.VIEW} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { mode: MODES.VIEW },
},
{
path: "/alerts/:alertId([0-9]+)/edit",
title: "Alert",
render: routeParams => (
<AuthenticatedPageWrapper key={`/alerts/${routeParams.alertId}`}>
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to duplicate this piece of logic (AuthenticatedPage + ErrorBoundry) on every page?

Copy link
Member

Choose a reason for hiding this comment

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

Also, passing handleError to every page feels like too much. Can't we solve this with some global state/context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I mentioned in reply to @gabrieldutra, I'm going to revisit this and few other things in a follow-up PR; currently it's stable, and I prefer not to add any more changes to this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, passing handleError to every page feels like too much. Can't we solve this with some global state/context?

Yes, and that's what I plan to do. There is a context already though; it's easy to use it with function-based components and a bit trickier with class-based ones - I didn't want to deal with it now so just passed it as a prop everywhere.

<ErrorBoundaryContext.Consumer>
{({ handleError }) => <AlertPage {...routeParams} onError={handleError} />}
{({ handleError }) => <AlertPage {...currentRoute.routeParams} mode={MODES.EDIT} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { mode: MODES.EDIT },
},
];
11 changes: 7 additions & 4 deletions client/app/pages/alerts/AlertsList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,17 @@ const AlertsListPage = liveItemsList(
export default {
path: "/alerts",
title: "Alerts",
render: (routeParams, currentRoute) => (
<AuthenticatedPageWrapper key="/alerts">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => (
<AlertsListPage routeParams={routeParams} currentRoute={currentRoute} onError={handleError} />
<AlertsListPage
routeParams={{ ...currentRoute.routeParams, currentPage: "alerts" }}
currentRoute={currentRoute}
onError={handleError}
/>
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "alerts" },
};
22 changes: 14 additions & 8 deletions client/app/pages/dashboards/DashboardList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,29 +152,35 @@ export default [
{
path: "/dashboards",
title: "Dashboards",
render: (routeParams, currentRoute) => (
<AuthenticatedPageWrapper key="/dashboards">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => (
<DashboardListPage routeParams={routeParams} currentRoute={currentRoute} onError={handleError} />
<DashboardListPage
routeParams={{ ...currentRoute.routeParams, currentPage: "all" }}
currentRoute={currentRoute}
onError={handleError}
/>
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "all" },
},
{
path: "/dashboards/favorites",
title: "Favorite Dashboards",
render: (routeParams, currentRoute) => (
<AuthenticatedPageWrapper key="/dashboards/favorites">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => (
<DashboardListPage routeParams={routeParams} currentRoute={currentRoute} onError={handleError} />
<DashboardListPage
routeParams={{ ...currentRoute.routeParams, currentPage: "favorites" }}
currentRoute={currentRoute}
onError={handleError}
/>
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "favorites" },
},
];
6 changes: 3 additions & 3 deletions client/app/pages/dashboards/DashboardPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,10 +412,10 @@ DashboardPage.defaultProps = {

export default {
path: "/dashboard/:dashboardSlug",
render: routeParams => (
<AuthenticatedPageWrapper key={`/dashboard/${routeParams.dashboardSlug}`}>
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <DashboardPage {...routeParams} onError={handleError} />}
{({ handleError }) => <DashboardPage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's that worth it at this point, but it looks like pages follow the same pattern when being defined, if this is not intended to be changed (and every page should follow this notation), we perhaps could abstract it a bit more with a closure returning this function? (seems we can tweak it to receive only the page component and some extra routeParams):

{
  path: "/dashboard/:dashboardSlug",
  render: authenticatedPage(DashboardPage, { extraData: "dashboard-page" }),
}

BTW, if I had to choose, I think it's better to pass routeParams as separate props (like you did to List Pages) and not directly to the props (I don't know, but it feels risky to me to leave props that open 😬).

Well, lmk your opinion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Yes, indeed it makes sense. But let's do it in a follow-up PR - I don't want to add anything more to this one (except of bug fixes related to PR's subject);
  2. With new router it's safe to spread route params - that object contains only values from URL (in this example - dashboardSlug) and resolved values (which will be removed soon), Router does not add anything to it (unlike Angular's router which was merging params from URL, resolved values and query string params).

Expand Down
6 changes: 3 additions & 3 deletions client/app/pages/dashboards/PublicDashboardPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ class PublicDashboardPage extends React.Component {
export default {
path: "/public/dashboards/:token",
authenticated: false,
render: routeParams => (
<SignedOutPageWrapper key={`/public/dashboards/${routeParams.token}`} apiKey={routeParams.token}>
render: currentRoute => (
<SignedOutPageWrapper key={currentRoute.key} apiKey={currentRoute.routeParams.token}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <PublicDashboardPage {...routeParams} onError={handleError} />}
{({ handleError }) => <PublicDashboardPage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</SignedOutPageWrapper>
),
Expand Down
21 changes: 8 additions & 13 deletions client/app/pages/data-sources/DataSourcesList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,30 +162,25 @@ export default [
{
path: "/data_sources",
title: "Data Sources",
render: routeParams => (
<AuthenticatedPageWrapper key="/data_sources">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <DataSourcesListPage {...routeParams} onError={handleError} />}
{({ handleError }) => <DataSourcesListPage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: {
currentPage: "data_sources",
},
},
{
path: "/data_sources/new",
title: "Data Sources",
render: routeParams => (
<AuthenticatedPageWrapper key="/data_sources">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <DataSourcesListPage {...routeParams} onError={handleError} />}
{({ handleError }) => (
<DataSourcesListPage {...currentRoute.routeParams} isNewDataSourcePage onError={handleError} />
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: {
currentPage: "data_sources",
isNewDataSourcePage: true,
},
},
];
6 changes: 3 additions & 3 deletions client/app/pages/data-sources/EditDataSource.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ const EditDataSourcePage = wrapSettingsTab(null, EditDataSource);
export default {
path: "/data_sources/:dataSourceId([0-9]+)",
title: "Data Sources",
render: routeParams => (
<AuthenticatedPageWrapper key={`/data_sources/${routeParams.dataSourceId}`}>
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <EditDataSourcePage {...routeParams} onError={handleError} />}
{({ handleError }) => <EditDataSourcePage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
Expand Down
19 changes: 8 additions & 11 deletions client/app/pages/destinations/DestinationsList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,28 +150,25 @@ export default [
{
path: "/destinations",
title: "Alert Destinations",
render: routeParams => (
<AuthenticatedPageWrapper key="/destinations">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <DestinationsListPage {...routeParams} onError={handleError} />}
{({ handleError }) => <DestinationsListPage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: { currentPage: "destinations" },
},
{
path: "/destinations/new",
title: "Alert Destinations",
render: routeParams => (
<AuthenticatedPageWrapper key="/destinations">
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <DestinationsListPage {...routeParams} onError={handleError} />}
{({ handleError }) => (
<DestinationsListPage {...currentRoute.routeParams} isNewDestinationPage onError={handleError} />
)}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
resolve: {
key: "destinations",
isNewDestinationPage: true,
},
},
];
6 changes: 3 additions & 3 deletions client/app/pages/destinations/EditDestination.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ const EditDestinationPage = wrapSettingsTab(null, EditDestination);
export default {
path: "/destinations/:destinationId([0-9]+)",
title: "Alert Destinations",
render: routeParams => (
<AuthenticatedPageWrapper key={`/destinations/${routeParams.destinationId}`}>
render: currentRoute => (
<AuthenticatedPageWrapper key={currentRoute.key}>
<ErrorBoundaryContext.Consumer>
{({ handleError }) => <EditDestinationPage {...routeParams} onError={handleError} />}
{({ handleError }) => <EditDestinationPage {...currentRoute.routeParams} onError={handleError} />}
</ErrorBoundaryContext.Consumer>
</AuthenticatedPageWrapper>
),
Expand Down
Loading