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

kravets-levko · 2020-01-07T12:13:12Z

What type of PR is this? (check all applicable)

Refactor

Description

Related Tickets & Documents

React migration #3071, closes #4159.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

No UI changes

arikfr · 2020-01-07T12:29:50Z

handle route.resolve map in the same manner as angular-router does;

I'm not sure we should keep this pattern. It's probably better to handle the load lifecycle in the page component itself.

handle tags globally similarly to what angular does.

Also not sure this worth implementing. We will need to do some extra work if we don't, but will end up with a "cleaner" and more explicit solution.

But we can start with both implemented (specially considering you already done the resolve) as a crutch to help with move the migration forward and once everything is working, make another PR that removes each of them and replaces it with better pattern.

kravets-levko · 2020-01-07T12:33:19Z

@arikfr Implementing both features is not that hard (as you mentioned, I already implemented route.resolve; handling tags globally is also another 15-20 LoC). So I'll start with that, and - as you mentioned - later we can discuss if we need that features and remove if necessary.

…n embed)

…ory library

…d stuff in ApplicationArea component

kravets-levko · 2020-01-19T17:45:10Z

@gabrieldutra Fixed 👍 I also refined code a bit, so please review it as well. Thank you!

gabrieldutra · 2020-01-20T02:01:16Z

client/app/pages/dashboards/DashboardPage.jsx

+  render: currentRoute => (
+    <AuthenticatedPageWrapper key={currentRoute.key}>
      <ErrorBoundaryContext.Consumer>
-        {({ handleError }) => <DashboardPage {...routeParams} onError={handleError} />}
+        {({ handleError }) => <DashboardPage {...currentRoute.routeParams} onError={handleError} />}
      </ErrorBoundaryContext.Consumer>
    </AuthenticatedPageWrapper>
  ),


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

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);

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

arikfr · 2020-01-20T11:52:41Z

client/app/components/ApplicationArea/AuthenticatedPageWrapper.jsx

+
+  useEffect(() => {
+    if (bodyClass) {
+      document.body.classList.toggle(bodyClass, true);


(Not blocking; just to understand if we can improve this further on)

Do we really need to set this class on the body element instead of one of the children that we can control w/ React?

ATM - yes, because it stretches few wrapper components when that class is set. To avoid this, we have to revisit some layout styles which I didn't want to do in this PR.

client/app/components/tags-control/TagsControl.jsx

arikfr · 2020-01-20T12:05:59Z

client/app/pages/alert/Alert.jsx

+    path: "/alerts/:alertId([0-9]+)/edit",
+    title: "Alert",
+    render: currentRoute => (
+      <AuthenticatedPageWrapper key={currentRoute.key}>


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

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

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.

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.

arikfr · 2020-01-20T12:10:35Z

client/app/pages/index.js

+  querySnippetsRoutes,
+  organizationSettingsRoutes,
+  usersListRoutes,
+  userProfileRoutes,


👍 good find.

Not something to spend on time now, but it's interesting to see how Next.js handles this. Because they have dynamic imports and produce optimized builds.

client/app/config/index.js

arikfr

One thing I noticed which is broken:

Start a new query.
Execute.
Click on New Visualization.

Unlike before, it's not saved and you can't save the viz.

kravets-levko · 2020-01-20T15:46:21Z

@arikfr Seems it was broken during Query pages migration (I reproduced it on master). I'll open separate PR for it as well, let's not block this one with this bug.

arikfr · 2020-01-20T15:56:10Z

Here's another one:

Open homepage.
Click on the "Now Param" dashboard.

You get every parameter update added to the history so going back feels like it's not working (you need to click lots of time until you get back to the homepage).

arikfr · 2020-01-20T17:50:37Z

I triggered another build.

kravets-levko · 2020-01-20T18:07:34Z

@arikfr That test was still not stable even after my previous fix; new fix worked fine on my PC, need to try on CI now.

A short explanation about history bloating bug fix: the actual issue is in useDashboard hook and it's not fixed here, because it's not related to a router itself and also requires it's own proper fix and discussion (PR is coming soon). In short: every time dashboard it updated, useDashboard decides to update URL as well; when refreshing dashboard - each widget updates dashboard twice - therefore so much history entries. I'm not sure how it worked with $location service (probably it updates URL only if something really changes). But since that updates are not necessary - they should not happen at all.

Also, while reproducing this bug on master, I noticed that when changing param values manually (or update other query string params), and then hitting Back button in browser - URL gets changed, but page does not update. It happens on dashboard page, all list pages and on query pages. So I decided to just replace URL (not push new history entry) - it (a sort of) fixed both bugs: history is not flooded with entries that pages cannot handle anyway.

arikfr · 2020-01-20T18:57:13Z

🙌

jezdez · 2020-01-22T10:10:53Z

Congrats!!

Migrate router and <app-view> to React: skeleton

ee529fe

kravets-levko added Frontend Frontend: React Frontend codebase migration to React labels Jan 7, 2020

kravets-levko self-assigned this Jan 7, 2020

arikfr mentioned this pull request Jan 7, 2020

React migration #3071

Closed

Update layout on route change

94e3de1

kravets-levko added 4 commits January 8, 2020 14:35

Start moving page routes from angular to react

0e85dd8

Move page routes to react except of public dashboard and visualizatio…

ce64f0d

…n embed)

Move public dashboard and visualization embed routes to React

4058093

Replace $route/$routeParams usages

b4ff41c

kravets-levko force-pushed the migrate-router-to-react branch from 61fbc54 to b4ff41c Compare January 8, 2020 15:33

kravets-levko added 4 commits January 8, 2020 18:02

Some cleanup

d874669

Replace AngularJS $location service with implementation based on hist…

8bb6c6d

…ory library

Minor fix to how ApplicationView handles route change

e4350f0

Explicitly use global layout for each page instead of handling relate…

933b50a

…d stuff in ApplicationArea component

kravets-levko force-pushed the migrate-router-to-react branch from a65f5f5 to 933b50a Compare January 10, 2020 19:03

Error handling

d2e0f98

gabrieldutra mentioned this pull request Jan 12, 2020

Replace $http with axios #4497

Merged

22 tasks

weekly-digest bot mentioned this pull request Jan 13, 2020

Weekly Digest (6 January, 2020 - 13 January, 2020) #4539

Closed

kravets-levko added 2 commits January 13, 2020 13:59

Merge branch 'master' into migrate-router-to-react

824ef59

Remove AngularJS and related dependencies

b2b504e

getredash deleted a comment from srs231198 Jan 13, 2020

gabrieldutra and others added 4 commits January 13, 2020 14:20

Move Parameter factory method to a separate file

9815996

Fix CSS (replace custom components with classes)

7306d21

Fix: keep other url parts when updating location partially; refine code

b140b55

Merge branch 'master' into migrate-router-to-react

e075d40

kravets-levko force-pushed the migrate-router-to-react branch from e9a3311 to 6e8872c Compare January 14, 2020 13:46

Fix tests

4f299fe

kravets-levko added 4 commits January 19, 2020 18:28

Use currentRoute.key instead of hard-coded keys for page components

dc4051a

Tidy up Router

a54d917

Tidy up location service

d822d39

Fix tests

a92d1fa

kravets-levko requested a review from gabrieldutra January 19, 2020 17:45

gabrieldutra reviewed Jan 20, 2020

View reviewed changes

Merge branch 'master' into migrate-router-to-react

2755e12

gabrieldutra approved these changes Jan 20, 2020

View reviewed changes

arikfr reviewed Jan 20, 2020

View reviewed changes

This was referenced Jan 20, 2020

Change visualizations import to be static #4565

Closed

Refactor showModal to avoid having catch everywhere #4566

Closed

arikfr requested changes Jan 20, 2020

View reviewed changes

kravets-levko force-pushed the migrate-router-to-react branch 2 times, most recently from 62e2cc6 to 9d00be5 Compare January 20, 2020 16:58

restyled-io bot mentioned this pull request Jan 20, 2020

Restyle Migrate router and <app-view> to React #4568

Closed

Don't add parameters changes to browser's history

f8e7334

kravets-levko force-pushed the migrate-router-to-react branch from 9d00be5 to f8e7334 Compare January 20, 2020 17:11

arikfr approved these changes Jan 20, 2020

View reviewed changes

Fix test (improved fix)

7e3f9cf

kravets-levko force-pushed the migrate-router-to-react branch from ff4e311 to 7e3f9cf Compare January 20, 2020 18:29

kravets-levko merged commit a682265 into master Jan 20, 2020

kravets-levko deleted the migrate-router-to-react branch January 20, 2020 18:56

weekly-digest bot mentioned this pull request Jan 27, 2020

Weekly Digest (20 January, 2020 - 27 January, 2020) #4591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kravets-levko commented Jan 7, 2020 •

edited

Loading

arikfr commented Jan 7, 2020

kravets-levko commented Jan 7, 2020

kravets-levko commented Jan 19, 2020

gabrieldutra Jan 20, 2020

kravets-levko Jan 20, 2020

arikfr Jan 20, 2020

kravets-levko Jan 20, 2020

arikfr Jan 20, 2020

arikfr Jan 20, 2020

kravets-levko Jan 20, 2020

kravets-levko Jan 20, 2020

arikfr Jan 20, 2020

arikfr left a comment

kravets-levko commented Jan 20, 2020

arikfr commented Jan 20, 2020

arikfr commented Jan 20, 2020

kravets-levko commented Jan 20, 2020

arikfr commented Jan 20, 2020

jezdez commented Jan 22, 2020

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

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

Conversation

kravets-levko commented Jan 7, 2020 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

arikfr commented Jan 7, 2020

kravets-levko commented Jan 7, 2020

kravets-levko commented Jan 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr left a comment

Choose a reason for hiding this comment

kravets-levko commented Jan 20, 2020

arikfr commented Jan 20, 2020

arikfr commented Jan 20, 2020

kravets-levko commented Jan 20, 2020

arikfr commented Jan 20, 2020

jezdez commented Jan 22, 2020

kravets-levko commented Jan 7, 2020 •

edited

Loading