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.

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

Routing Refactor #717

Merged
merged 63 commits into from
May 7, 2020
Merged

Routing Refactor #717

merged 63 commits into from
May 7, 2020

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented May 5, 2020

Changes

This is turning into a pretty lofty PR and is still Work In Progress. Things done so far:

  1. Replace react-router with kea-router to have all routing logic in one place. This makes all the actionToUrl and urlToAction functions work much better and all logic about the state of the app is also now in one place (kea).
  2. Implement code splitting. Since I had to redo the App.js <Router /> code anyway, I just added code splitting as well. Loading of the different scenes is also quite intelligent. When you open a new scene by changing the URL (/dashboard -> /people), we start loading it in the background. If it is loaded within 0.5sec, we show it directly. Only if it takes longer than half a second to load, do we show a spinner. This makes the app feel much snappier.
  3. Fix the events table filtering and related URLs (issue in Can not go back from event searches #688). Now the events table and the property filters synchronise their state through the URL.
  4. Fix the action/edit table filtering (remove the filter).
  5. Show events under actions/edit even before it's saved, update the list of events reliably once the action is saved
  6. Fix similar issues with filter and URL synchronisation on the /person/ID page

Still todo:

  • Live actions page URL & filters
  • Trends filters (the 3 back button issue from It takes three clicks to go back to dashboard #687)
  • Trends page properties don't sync with the URL properly (clicking back does not update the page)
  • Action Filter new action issue
  • Bug (just introduced) when opening cohorts
  • Funnel actions/events & properties
  • Trends set $pageview if empty when clicking on submenu
  • ? Saving cohorts doesn't update list (bug is also on master)
  • ? Use searchParams when searching for people
  • ? Use searchParams for filtering on paths
  • ? Show loader on paths when changing the dates
  • Testing, testing, testing
    • Trends
    • Trends default $pageview
    • Dashboard
    • Events
    • Actions
    • New Action
    • Actions Live
    • People / All Users + Search
    • Cohorts
    • New Cohort
    • Funnels / New / Edit
    • Paths
    • Setup

Maybe:

  1. Check if I could also integrate Can't have multiple filters with the same key #714 here
  2. Change the URL also when searching on the /people page

Checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)

@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 09:46 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 12:40 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 12:47 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 15:22 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 16:08 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 16:43 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 5, 2020 23:00 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 6, 2020 12:56 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 6, 2020 13:43 Inactive
@mariusandra mariusandra requested a review from timgl May 6, 2020 13:46
@mariusandra
Copy link
Collaborator Author

@timgl this is ready for review! @EDsCODE in case you have time, you may check it as well... if now, please at least read the parts about kea-router below, as these will be useful going forward.

I managed to find, fix and clean up a lot of stuff as I went through the files. This includes a lot of == --> === style fixes, cleaning up various React key warnings, let -> const, etc.

I untangled a lot of logic regarding the routes, filters, etc and gave things a much stronger foundation. Now using kea-router, it's joyful to use the URL search parameters to store and all sorts of filters.

I also removed the darkness from the loading screens and used the default antd spinner.

At the end, I went through and tested all the pages I could find in the menu and clicked all the buttons I could find... and things work. That said, it's possible I might have mixed something, so a thorough QA is very much appreciated.

Now, at least for me, when I move around posthog, all links, filters, etc work the way they should, sparking unimaginable joy! :D

This Sparks Joy

@mariusandra mariusandra changed the title Routing Refactor [WIP] Routing Refactor May 6, 2020
Copy link
Collaborator

@timgl timgl left a comment

Choose a reason for hiding this comment

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

Slight meta point: all of this loading looks like it doesn't always correctly de-duplicates the css and js? Some of these files are still a couple hundred kbs
image
Maybe that is right? It's not a blocker for merging I think

</tr>
</thead>
<tbody>
{isLoading && <TableRowLoading colSpan={5} />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

image
I'm not sure this looks that great. Events endpoints are also fairly quick so it's slightly flicker-y. would be better if it was an overlay I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I didn't test changing filters in this table after changing the loaders, even though it's quite an obvious issue. I fixed it now for this table and for the others: if there are any rows already shown in the table, the loader is an overlay. Otherwise it's like on your screenshot, except of course without the table rows.

For added joy, I implemented a feature for the events table, which will show the loading overlay only after it has been loading for 500ms. Thus it's possible most people will never even see a loader anymore, except on the initial page load.

I can change that delay in case you have some better number to suggest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good

<div />
<Spin />
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no loader when changing the date. willing to accept out of scope for 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.

That scene could use a good refactor :). I'd say it's outside the scope of this PR, as nothing changed on Paths from before, except just the addition of the spinner on the (shown-once) loading screen.

@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 6, 2020 19:24 Inactive
@mariusandra
Copy link
Collaborator Author

Hey, regarding the bundle splitting, yeah, we should finetune it manually later. However webpack's defaults already improve the downloaded JS size quite a bit.

  • Before (on app.posthog.com) every page loaded 1.8MB of JS. That's the minified & non-gzipped size.
  • Now on the heroku branch, when I open the events page, it loads 1MB of JS. The trends page loads 1.6MB. The dashboard loads 1.1Mb.

I guess it makes a difference and we can adjust the splitting later to be more precise.

@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 7, 2020 07:28 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 7, 2020 07:30 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 7, 2020 08:15 Inactive
@timgl timgl temporarily deployed to posthog-688-events-tabl-betmrb May 7, 2020 08:28 Inactive
@mariusandra
Copy link
Collaborator Author

mariusandra commented May 7, 2020

I found a few more bugs that got squashed:

  1. The table row loading indicator actually crashed some pages (actions.length > 0, but actions === undefined). Those are fixed now.
  2. The date filter with custom dates (YYYY-MM-DD) didn't work. I updated the <DateFilter> to just return days instead of a complex momentJS object, which got converted to YYYY-MM-DD anyway before being put in the URL. I tested all 3 places with date filters (paths, trends, funnels) and it worked.

Plus one extra feature: I removed the toParams() calls and instead pipe various paths when needed through the router, which handles param serialisation for us in a predictable way. I added a new function combineUrl to the router, which helps create sane urls easily if we ever need to merge props on the fly, like in the FilterPropertyLinks.

Copy link
Collaborator

@timgl timgl left a comment

Choose a reason for hiding this comment

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

LGTM

@mariusandra mariusandra merged commit 6aea092 into master May 7, 2020
@mariusandra mariusandra deleted the 688-events-table-back-button branch May 7, 2020 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants