Skip to content

Commit

Permalink
fix: Dehydration of class components (deephaven#1535)
Browse files Browse the repository at this point in the history
  • Loading branch information
mattrunyon authored Sep 22, 2023
1 parent bd08e1f commit 3e834de
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 6 deletions.
36 changes: 31 additions & 5 deletions packages/dashboard/src/DashboardLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import PanelEvent from './PanelEvent';
import { GLPropTypes, useListener } from './layout';
import { getDashboardData, updateDashboardData } from './redux';
import {
isWrappedComponent,
PanelComponentType,
PanelDehydrateFunction,
PanelHydrateFunction,
Expand Down Expand Up @@ -118,24 +119,49 @@ export function DashboardLayout({
componentDehydrate
);

function wrappedComponent(props: PanelProps): JSX.Element {
const CType = componentType;
function wrappedComponent(
props: PanelProps,
ref: React.Ref<unknown>
): JSX.Element {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const CType = componentType as any;
const PanelWrapperType = panelWrapper;

/*
Checking for class components will let us silence the React warning
about assigning refs to function components not using forwardRef.
The ref is used to detect changes to class component state so we
can track changes to panelState. We should opt for more explicit
state changes in the future and in functional components.
*/
const isClassComponent =
(isWrappedComponent(CType) &&
CType.WrappedComponent.prototype != null &&
CType.WrappedComponent.prototype.isReactComponent != null) ||
(CType.prototype != null && CType.prototype.isReactComponent != null);

// Props supplied by GoldenLayout
const { glContainer, glEventHub } = props;
return (
<PanelErrorBoundary glContainer={glContainer} glEventHub={glEventHub}>
{/* eslint-disable-next-line react/jsx-props-no-spreading */}
<PanelWrapperType {...props}>
{/* eslint-disable-next-line react/jsx-props-no-spreading */}
<CType {...props} />
{isClassComponent ? (
// eslint-disable-next-line react/jsx-props-no-spreading
<CType {...props} ref={ref} />
) : (
// eslint-disable-next-line react/jsx-props-no-spreading
<CType {...props} />
)}
</PanelWrapperType>
</PanelErrorBoundary>
);
}

const cleanup = layout.registerComponent(name, wrappedComponent);
const cleanup = layout.registerComponent(
name,
React.forwardRef(wrappedComponent)
);
hydrateMap.set(name, componentHydrate);
dehydrateMap.set(name, componentDehydrate);
return cleanup;
Expand Down
11 changes: 11 additions & 0 deletions packages/golden-layout/src/utils/ReactComponentHandler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,17 @@ export default class ReactComponentHandler {
var defaultProps = {
glEventHub: this._container.layoutManager.eventHub,
glContainer: this._container,
/**
* This ref assignment makes use of callback refs which is a legacy ref style in React.
* It uses the callback to inject GoldenLayout _onUpdate into the React componentWillUpdate lifecycle.
* This allows GoldenLayout to track the internal state of class components.
* We then emit this state change and somewhere furter up, serialize it.
* Specifically we look for state.panelState changes.
* We should not do this going forward as it's quite unclear where/why your component's state might be saved.
* This ref cannot be removed unless we refactor all existing panels to not rely on the magic of panelState.
* DashboardUtils#dehydrate is where the panelState gets read/saved.
*/
ref: this._gotReactComponent.bind(this),
};
var props = $.extend(defaultProps, this._container._config.props);
return React.createElement(this._reactClass, props);
Expand Down
15 changes: 14 additions & 1 deletion tests/notebook.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { test, expect } from '@playwright/test';
import shortid from 'shortid';
import { pasteInMonaco } from './utils';

test('test creating a file, saving it, closing it, re-opening it, running it, then deleting it', async ({
test('test creating a file, saving it, reloading the page, closing it, re-opening it, running it, then deleting it', async ({
page,
}) => {
await page.goto('');
Expand Down Expand Up @@ -33,6 +33,19 @@ test('test creating a file, saving it, closing it, re-opening it, running it, th
// Click button:has-text("Save")
await page.locator('button:has-text("Save")').click();

// Pause so the save can actually finish
// We eagerly show the save status, so can't use that to detect save finish
await new Promise(resolve => {
setTimeout(resolve, 2000);
});

// Reload page to check state of panel is saved
await page.reload();

await expect(
page.locator('.editor-container').locator('textarea')
).not.toBeEmpty();

// Click close on the notebook file .lm_close_tab
await page.locator('.lm_close_tab').click();

Expand Down

0 comments on commit 3e834de

Please sign in to comment.