Skip to content

Commit

Permalink
refactor: Fix fast refresh invalidations (#1150)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mattrunyon committed Mar 17, 2023
1 parent 362928f commit 2606826
Show file tree
Hide file tree
Showing 53 changed files with 392 additions and 304 deletions.
19 changes: 18 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/code-studio/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import logInit from './log/LogInit';

logInit();

// eslint-disable-next-line react-refresh/only-export-components
const AppRoot = React.lazy(() => import('./AppRoot'));

ReactDOM.render(
Expand Down
4 changes: 3 additions & 1 deletion packages/code-studio/src/main/AppInit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ const mapStateToProps = (state: RootState) => ({
workspaceStorage: getWorkspaceStorage(state),
});

export default connect(mapStateToProps, {
const ConnectedAppInit = connect(mapStateToProps, {
setActiveTool: setActiveToolAction,
setCommandHistoryStorage: setCommandHistoryStorageAction,
setDashboardData: setDashboardDataAction,
Expand All @@ -391,3 +391,5 @@ export default connect(mapStateToProps, {
setWorkspaceStorage: setWorkspaceStorageAction,
setServerConfigValues: setServerConfigValuesAction,
})(AppInit);

export default ConnectedAppInit;
4 changes: 3 additions & 1 deletion packages/code-studio/src/main/AppMainContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -904,8 +904,10 @@ const mapStateToProps = (state: RootState) => ({
serverConfigValues: getServerConfigValues(state),
});

export default connect(mapStateToProps, {
const ConnectedAppMainContainer = connect(mapStateToProps, {
setActiveTool: setActiveToolAction,
updateDashboardData: updateDashboardDataAction,
updateWorkspaceData: updateWorkspaceDataAction,
})(withRouter(AppMainContainer));

export default ConnectedAppMainContainer;
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,8 @@ const mapStateToProps = (state: RootState) => ({
settings: getSettings(state),
});

export default connect(mapStateToProps, { saveSettings: saveSettingsAction })(
ColumnSpecificSectionContent
);
const ConnectedColumnSpecificSectionContent = connect(mapStateToProps, {
saveSettings: saveSettingsAction,
})(ColumnSpecificSectionContent);

export default ConnectedColumnSpecificSectionContent;
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,8 @@ const mapStateToProps = (state: RootState) => ({
settings: getSettings(state),
});

export default connect(mapStateToProps, { saveSettings: saveSettingsAction })(
FormattingSectionContent
);
const ConnectedFormattingSectionContent = connect(mapStateToProps, {
saveSettings: saveSettingsAction,
})(FormattingSectionContent);

export default ConnectedFormattingSectionContent;
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ const mapStateToProps = (state: RootState) => ({

const mapDispatchToProps = { saveSettings: saveSettingsAction };

export default connect(
const ConnectedShortcutSectionContent = connect(
mapStateToProps,
mapDispatchToProps
)(ShortcutSectionContent);

export default ConnectedShortcutSectionContent;
4 changes: 3 additions & 1 deletion packages/code-studio/src/styleguide/StyleGuideInit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const mapStateToProps = (state: RootState) => ({
workspace: getWorkspace(state),
});

export default connect(mapStateToProps, {
const ConnectedStyleGuideInit = connect(mapStateToProps, {
setWorkspace: setWorkspaceAction,
})(StyleGuideInit);

export default ConnectedStyleGuideInit;
1 change: 1 addition & 0 deletions packages/code-studio/src/styleguide/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import logInit from '../log/LogInit';

logInit();

// eslint-disable-next-line react-refresh/only-export-components
const StyleGuideRoot = React.lazy(() => import('./StyleGuideRoot'));

ReactDOM.render(
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/DateTimeInput.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import DateTimeInput, { addSeparators } from './DateTimeInput';
import DateTimeInput from './DateTimeInput';
import { addSeparators } from './DateTimeInputUtils';

const DEFAULT_DATE_TIME = '2022-02-22 00:00:00.000000000';
// Zero width space
Expand Down
12 changes: 4 additions & 8 deletions packages/components/src/DateTimeInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import classNames from 'classnames';
import Log from '@deephaven/log';
import MaskedInput, { SelectionSegment } from './MaskedInput';
import { getNextSegmentValue } from './DateInputUtils';
import { addSeparators } from './DateTimeInputUtils';

const log = Log.module('DateTimeInput');

Expand All @@ -26,7 +27,7 @@ type DateTimeInputProps = {
'data-testid'?: string;
};

export function fixIncompleteValue(value: string): string {
function fixIncompleteValue(value: string): string {
if (value != null && value.length >= DATE_VALUE_STRING.length) {
return `${value.substring(0, DATE_VALUE_STRING.length)}${value
.substring(DATE_VALUE_STRING.length)
Expand All @@ -35,15 +36,10 @@ export function fixIncompleteValue(value: string): string {
return value;
}

export function addSeparators(value: string): string {
const dateTimeMillis = value.substring(0, 23);
const micros = value.substring(23, 26);
const nanos = value.substring(26);
return [dateTimeMillis, micros, nanos].filter(v => v !== '').join('\u200B');
function removeSeparators(value: string): string {
return value.replace(/\u200B/g, '');
}

const removeSeparators = (value: string) => value.replace(/\u200B/g, '');

const EXAMPLES = [addSeparators(DEFAULT_VALUE_STRING)];

const DateTimeInput = React.forwardRef<HTMLInputElement, DateTimeInputProps>(
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/DateTimeInputUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* eslint-disable import/prefer-default-export */
export function addSeparators(value: string): string {
const dateTimeMillis = value.substring(0, 23);
const micros = value.substring(23, 26);
const nanos = value.substring(26);
return [dateTimeMillis, micros, nanos].filter(v => v !== '').join('\u200B');
}
3 changes: 2 additions & 1 deletion packages/components/src/MaskedInput.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import { render } from '@testing-library/react';
import MaskedInput, { fillToLength, trimTrailingMask } from './MaskedInput';
import MaskedInput from './MaskedInput';
import { fillToLength, trimTrailingMask } from './MaskedInputUtils';

const TIME_PATTERN = '([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]';

Expand Down
52 changes: 5 additions & 47 deletions packages/components/src/MaskedInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import React, { useMemo, useEffect, useCallback } from 'react';
import classNames from 'classnames';
import Log from '@deephaven/log';
import { useForwardedRef } from '@deephaven/react-hooks';
import {
DEFAULT_GET_PREFERRED_REPLACEMENT_STRING,
fillToLength,
trimTrailingMask,
} from './MaskedInputUtils';
import './MaskedInput.scss';

const log = Log.module('MaskedInput');
Expand All @@ -18,53 +23,6 @@ const SELECTION_DIRECTION = {
*/
const FIXED_WIDTH_SPACE = '\u2007';

export function DEFAULT_GET_PREFERRED_REPLACEMENT_STRING(
value: string,
replaceIndex: number,
newChar: string
): string {
return (
value.substring(0, replaceIndex) +
newChar +
value.substring(replaceIndex + 1)
);
}

/**
* Fill the string on the right side with the example value to the given length
* @param checkValue Initial string to pad
* @param exampleValue Example value
* @param length Target length
* @returns String padded with the given example value
*/
export function fillToLength(
checkValue: string,
exampleValue: string,
length: number
): string {
return checkValue.length < length
? `${checkValue}${exampleValue.substring(checkValue.length, length)}`
: checkValue;
}

/**
* Trim all characters matching the empty mask on the right side of the given value
* @param value String to trim
* @param emptyMask Empty mask
* @returns Trimmed string
*/
export function trimTrailingMask(value: string, emptyMask: string): string {
let { length } = value;
for (let i = value.length - 1; i >= 0; i -= 1) {
if (emptyMask[i] === value[i]) {
length = i;
} else {
break;
}
}
return value.substring(0, length);
}

export type SelectionSegment = {
selectionStart: number;
selectionEnd: number;
Expand Down
47 changes: 47 additions & 0 deletions packages/components/src/MaskedInputUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/* eslint-disable import/prefer-default-export */
export function DEFAULT_GET_PREFERRED_REPLACEMENT_STRING(
value: string,
replaceIndex: number,
newChar: string
): string {
return (
value.substring(0, replaceIndex) +
newChar +
value.substring(replaceIndex + 1)
);
}

/**
* Fill the string on the right side with the example value to the given length
* @param checkValue Initial string to pad
* @param exampleValue Example value
* @param length Target length
* @returns String padded with the given example value
*/
export function fillToLength(
checkValue: string,
exampleValue: string,
length: number
): string {
return checkValue.length < length
? `${checkValue}${exampleValue.substring(checkValue.length, length)}`
: checkValue;
}

/**
* Trim all characters matching the empty mask on the right side of the given value
* @param value String to trim
* @param emptyMask Empty mask
* @returns Trimmed string
*/
export function trimTrailingMask(value: string, emptyMask: string): string {
let { length } = value;
for (let i = value.length - 1; i >= 0; i -= 1) {
if (emptyMask[i] === value[i]) {
length = i;
} else {
break;
}
}
return value.substring(0, length);
}
6 changes: 2 additions & 4 deletions packages/components/src/TimeInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@ import React, {
} from 'react';
import Log from '@deephaven/log';
import { TimeUtils } from '@deephaven/utils';
import MaskedInput, {
DEFAULT_GET_PREFERRED_REPLACEMENT_STRING,
SelectionSegment,
} from './MaskedInput';
import MaskedInput, { SelectionSegment } from './MaskedInput';
import { DEFAULT_GET_PREFERRED_REPLACEMENT_STRING } from './MaskedInputUtils';

export type { SelectionSegment } from './MaskedInput';

Expand Down
1 change: 1 addition & 0 deletions packages/components/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export { default as DropdownMenu } from './menu-actions';
export * from './menu-actions';
export { default as MaskedInput } from './MaskedInput';
export * from './MaskedInput';
export * from './MaskedInputUtils';
export * from './navigation';
export { default as Option } from './Option';
export * from './popper';
Expand Down
2 changes: 1 addition & 1 deletion packages/dashboard-core-plugins/src/ConsolePlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export type ConsolePluginProps = DashboardPluginComponentProps & {
notebooksUrl: string;
};

export function assertIsConsolePluginProps(
function assertIsConsolePluginProps(
props: Partial<ConsolePluginProps>
): asserts props is ConsolePluginProps {
assertIsDashboardPluginProps(props);
Expand Down
2 changes: 1 addition & 1 deletion packages/dashboard-core-plugins/src/GridPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Table, VariableDefinition } from '@deephaven/jsapi-shim';
import shortid from 'shortid';
import { IrisGridPanel, IrisGridPanelProps } from './panels';

export const SUPPORTED_TYPES: string[] = [
const SUPPORTED_TYPES: string[] = [
dh.VariableType.TABLE,
dh.VariableType.TREETABLE,
dh.VariableType.HIERARCHICALTABLE,
Expand Down
4 changes: 3 additions & 1 deletion packages/dashboard-core-plugins/src/linker/Linker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -748,4 +748,6 @@ export class Linker extends Component<LinkerProps, LinkerState> {
}
}

export default connector(Linker);
const ConnectedLinker = connector(Linker);

export default ConnectedLinker;
10 changes: 4 additions & 6 deletions packages/dashboard-core-plugins/src/panels/ChartPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import ChartColumnSelectorOverlay, {
import './ChartPanel.scss';
import { Link } from '../linker/LinkerUtils';
import { PanelState as IrisGridPanelState } from './IrisGridPanel';
import { isChartPanelTableMetadata } from './ChartPanelUtils';
import { ColumnSelectionValidator } from '../linker/ColumnSelectionValidator';

const log = Log.module('ChartPanel');
Expand All @@ -76,11 +77,6 @@ export type FilterMap = Map<string, string>;

export type LinkedColumnMap = Map<string, { name: string; type: string }>;

export function isChartPanelTableMetadata(
metadata: ChartPanelMetadata
): metadata is ChartPanelTableMetadata {
return (metadata as ChartPanelTableMetadata).settings !== undefined;
}
export type ChartPanelFigureMetadata = {
figure: string;
};
Expand Down Expand Up @@ -1198,7 +1194,7 @@ const mapStateToProps = (
};
};

export default connect(
const ConnectedChartPanel = connect(
mapStateToProps,
{
setActiveTool: setActiveToolAction,
Expand All @@ -1207,3 +1203,5 @@ export default connect(
null,
{ forwardRef: true }
)(ChartPanel);

export default ConnectedChartPanel;
8 changes: 8 additions & 0 deletions packages/dashboard-core-plugins/src/panels/ChartPanelUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { ChartPanelMetadata, ChartPanelTableMetadata } from './ChartPanel';

// eslint-disable-next-line import/prefer-default-export
export function isChartPanelTableMetadata(
metadata: ChartPanelMetadata
): metadata is ChartPanelTableMetadata {
return (metadata as ChartPanelTableMetadata).settings !== undefined;
}

0 comments on commit 2606826

Please sign in to comment.