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

Improve HMR with Vite #727

Closed
mattrunyon opened this issue Aug 23, 2022 · 2 comments · Fixed by #1150
Closed

Improve HMR with Vite #727

mattrunyon opened this issue Aug 23, 2022 · 2 comments · Fixed by #1150
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@mattrunyon
Copy link
Collaborator

mattrunyon commented Aug 23, 2022

Switching from CRA to Vite also moves us off of Webpack. Webpack possibly does some heuristics to make HMR function in different cases where Vite does not HMR properly.

Vite does not HMR if there are items it cannot determine are side-effect free. Some items like shortcuts possibly break this with the current singleton implementation and should use a class instead.

Also, HOC may break fast refresh. I am not sure if our current redux setup breaks this since it uses HOC.

Any imports such as import 'myScript.js' or import 'myStyle.css' will also be considered as side-effects most likely since the only reason an import like that exists is to execute something in the file. Not sure if sass triggers this w/ Vite, but it wouldn't surprise me

Our log package could be an issue b/c in all the components we usually have a setup log line in the top level of the file which might be marked as a side-effect

vitejs/vite#4577 (comment)
vitejs/vite-plugin-react#21

@mattrunyon mattrunyon added enhancement New feature or request triage Issue requires triage labels Aug 23, 2022
@vbabich vbabich added this to the August 2022 milestone Aug 24, 2022
@vbabich vbabich removed the triage Issue requires triage label Aug 24, 2022
@mofojed mofojed modified the milestones: August 2022, September 2022 Sep 9, 2022
@mofojed mofojed modified the milestones: September 2022, October 2022 Oct 3, 2022
@mofojed mofojed modified the milestones: October 2022, November 2022 Nov 16, 2022
@mofojed mofojed modified the milestones: November 2022, December 2022 Dec 5, 2022
@mattrunyon
Copy link
Collaborator Author

https://github.com/ArnaudBarre/eslint-plugin-react-refresh

This might be a useful plugin for eslint to help spot some potential issues

@mofojed mofojed modified the milestones: December 2022, January 2023 Dec 28, 2022
@mofojed mofojed modified the milestones: January 2023, Backlog Jan 23, 2023
@mattrunyon
Copy link
Collaborator Author

Upgrading to Vite 4 provides a message in your console telling you when certain files cannot fast refresh. We should try to at least fix or keep a list of the known files. The dashboard core plugins seem to be invalid for fast refresh.

@mofojed mofojed modified the milestones: Backlog, March 2023 Mar 14, 2023
mattrunyon added a commit that referenced this issue Mar 17, 2023
Fixes #727 as best we can w/o requiring major changes. HMR works best w/
functional components and that's too big of a change to switch
everything to functional components just for HMR.

Added an eslint rule which will warn about things that will almost
certainly invalidate HMR. Fixed the warnings it emitted.

Tested locally by changing some displayed text values in some panels and
seeing if the page triggered a full reload. I didn't have any specific
files/cases that triggered full reloads previously and it seems Vite 4
has made it better on its own. If we start running into cases.

Saving `GridRenderer` doesn't trigger a full page reload (didn't before
either), but massively slows the page (also had this behavior prior to
this change). The change eventually propagates and refreshes

We should keep an eye on vitejs/vite#12062
which will likely also fix the slow HMR issues on some components. There
seems to be duplication of modules in the update list and it can explode
at times (like GridRenderer triggers 14 unique modules, but 20k updates
consisting of just those 14)

BREAKING CHANGE:
Renamed `renderFileListItem` to `FileListItem`.
Renamed `RenderFileListItemProps` to `FileListItemProps`.
Removed exports for `ConsolePlugin.assertIsConsolePluginProps`,
`GridPlugin.SUPPORTED_TYPES`, `FileList.getPathFromItem`,
`FileList.DRAG_HOVER_TIMEOUT`, `FileList.getItemIcon`,
`Grid.directionForKey`, `GotoRow.isIrisGridProxyModel`, and
`Aggregations.SELECTABLE_OPTIONS`. These were all only being consumed
within their own file and are not consumed in enterprise
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants