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

Toolbar Date Filter, remove react-hot-loader, fix dashboard date filter URL #3586

Merged
merged 33 commits into from
Mar 12, 2021

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Mar 5, 2021

Changes

This PR started innocently enough, with the desire to just add a datefilter into the toolbar, so we could now do this:

image

I had to refactor the DateFilter a bit to separate it from the dateFilterLogic (which is tied to the /insights url), and was happy when all was done. However even though all other pages were fine, the /insights and /dashboard/:id pages would not load anymore:

Screenshot 2021-03-05 at 09 05 05

The error was thrown when calling useState... inside the date filter? This took countless hours of debugging to track down 🤦, but in the end it's a conflict between the react hot module reloading system and multiple configurations inside webpack.json (we export an array of configs -- we can't merge them into one config with multiple entrypoints as the toolbar has a different set of webpack plugins that it needs for CSS).

Turns out we can't import the same "lib/components/*" inside both the toolbar and the main app... and have both running at the same time. Because the toolbar is loaded via posthog-js and comes with its own version of React inside the .js bundle, we'll have two copies of React loaded in memory... but both of them will try to load the same vendor~lib~datepicker-blabla.js file that's delivered by HMR, which is then linked to just one of the two React instances.

The only change to make this work was to remove the component from the toolbar:

Screenshot 2021-03-05 at 09 17 34

... but then there would be no date picker in the toolbar.

The solution (after trying oh so many things for, again, hours 😢), was to remove react hot module reloading altogether. It's deprecated anyway and we should switch to react fast refresh. Without react-hot-loader, when you make a change, then instead of a react component being swapped in place, the full page will reload instead. In my experience this HMR was in my experience often broken anyway and I frequently reloaded the entire page. Plus it didn't work if you modified a logic.

In addition to all of that, this PR also:

  • Upgrades to Webpack v5 (from v4)
  • Fixes the date filter on the dashboards page by disconnecting it from the URL (it would modify the URL when changed, but never read from the URL if reloaded)

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@mariusandra mariusandra requested review from timgl and Twixes March 5, 2021 12:08
@timgl timgl temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 12:11 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 12:23 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 12:59 Inactive
@mariusandra
Copy link
Collaborator Author

Cypress fails and it's going to be complicated to work around that: cypress-io/cypress#8948

@mariusandra mariusandra temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 14:02 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 14:14 Inactive
@mariusandra mariusandra had a problem deploying to posthog-toolbar-datefil-v9bpat March 5, 2021 14:23 Failure
@mariusandra mariusandra changed the title Toolbar Date Filter, Webpack 5, remove react-hot-loader, fix dashboard date filter URL Toolbar Date Filter, ~~Webpack 5~~, remove react-hot-loader, fix dashboard date filter URL Mar 5, 2021
@mariusandra mariusandra changed the title Toolbar Date Filter, ~~Webpack 5~~, remove react-hot-loader, fix dashboard date filter URL Toolbar Date Filter, remove react-hot-loader, fix dashboard date filter URL Mar 5, 2021
@mariusandra mariusandra had a problem deploying to posthog-toolbar-datefil-v9bpat March 5, 2021 14:27 Failure
@mariusandra mariusandra temporarily deployed to posthog-toolbar-datefil-v9bpat March 5, 2021 14:34 Inactive
@mariusandra
Copy link
Collaborator Author

Reverted back to webpack 4 because cypress. Tests are now green. Ready for a look.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

This is a big diff for such a small UX-wise change…

webpack.config.js Show resolved Hide resolved
package.json Show resolved Hide resolved
frontend/src/scenes/insights/Insights.js Outdated Show resolved Hide resolved
frontend/src/toolbar/stats/HeatmapStats.tsx Outdated Show resolved Hide resolved
frontend/src/toolbar/elements/heatmapLogic.ts Outdated Show resolved Hide resolved
@mariusandra
Copy link
Collaborator Author

Ready for another look!

@mariusandra mariusandra requested a review from Twixes March 12, 2021 12:41
@Twixes Twixes temporarily deployed to posthog-toolbar-datefil-u2b03b March 12, 2021 12:53 Inactive
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mariusandra
Copy link
Collaborator Author

Let's get it in!

@mariusandra mariusandra merged commit 8c98e4f into master Mar 12, 2021
@mariusandra mariusandra deleted the toolbar-datefilter branch March 12, 2021 13:54
@timgl timgl mentioned this pull request Mar 14, 2021
4 tasks
paolodamico added a commit that referenced this pull request Mar 15, 2021
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.

4 participants