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

Add React hot refresh and refine viz lib build #5104

Closed
wants to merge 3 commits into from

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented Aug 14, 2020

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

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other: build

Description

I was developing visualization editors in the dialog and was annoyed by automatic whole page refreshes and losing my opened dialog. So I decided to fix it and adding support for hot refresh!

The recently released react-refresh-webpack-plugin is amazing and actually very easy to setup, with some caveats that took me a couple of hours to debug.

To enable hot refresh, some additional changes had to be made:

  • Upgraded React, Webpack and Babel.
  • Move .eslintrc from client to the root directory, so it can apply to viz-lib, too. (VSCode ESLint check will fail in viz-lib because of missing .eslintrc)
  • Let the main webpack import source files in viz-lib directly. This is easier for debugging. (Sitenote: I also helped setup a similar configuration for Apache Superset).
  • Changed the import path alias for viz-lib from @ to @@, so it doesn't conflict with client/app. This is both a requirement of the aforementioned change and for reducing confusions
  • Fixed a couple of ESLint warnings in viz-lib

Related Tickets & Documents

#4837

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

@guidopetri
Copy link
Contributor

@ktmud , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@justinclift
Copy link
Member

Wow, this PR looks pretty huge. It'll probably take significant effort to rework / update (if still needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants