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

refactor: Fix fast refresh invalidations #1150

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Mar 14, 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

@mattrunyon mattrunyon requested a review from mofojed March 14, 2023 19:56
@mattrunyon mattrunyon self-assigned this Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #1150 (e1fcc6b) into main (a60bda5) will increase coverage by 1.03%.
The diff coverage is 88.42%.

@@            Coverage Diff             @@
##             main    #1150      +/-   ##
==========================================
+ Coverage   43.46%   44.50%   +1.03%     
==========================================
  Files         437      442       +5     
  Lines       32951    32961      +10     
  Branches     8305     8304       -1     
==========================================
+ Hits        14323    14670     +347     
+ Misses      18579    18242     -337     
  Partials       49       49              
Flag Coverage Δ
unit 44.50% <88.42%> (+1.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/index.tsx 0.00% <ø> (ø)
packages/code-studio/src/main/AppInit.tsx 0.00% <0.00%> (ø)
...udio/src/settings/ColumnSpecificSectionContent.tsx 98.73% <ø> (ø)
...e-studio/src/settings/FormattingSectionContent.tsx 63.41% <ø> (+0.29%) ⬆️
...ages/code-studio/src/styleguide/StyleGuideInit.tsx 0.00% <0.00%> (ø)
packages/code-studio/src/styleguide/index.tsx 0.00% <ø> (ø)
packages/components/src/MaskedInput.tsx 87.38% <ø> (-0.78%) ⬇️
packages/components/src/TimeInput.tsx 71.18% <ø> (ø)
...kages/dashboard-core-plugins/src/ConsolePlugin.tsx 20.92% <ø> (ø)
...s/dashboard-core-plugins/src/linker/LinkerUtils.ts 3.38% <ø> (ø)
... and 28 more

... and 19 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

packages/components/src/DateTimeInput.tsx Outdated Show resolved Hide resolved
packages/file-explorer/src/FileList.tsx Outdated Show resolved Hide resolved
packages/file-explorer/src/FileListContainer.tsx Outdated Show resolved Hide resolved
mofojed
mofojed previously approved these changes Mar 15, 2023
@mattrunyon mattrunyon merged commit 2606826 into deephaven:main Mar 17, 2023
@mattrunyon mattrunyon deleted the fast-refresh-fixes branch March 17, 2023 14:19
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve HMR with Vite
2 participants