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

fix: Dehydration of class components #1535

Merged
merged 3 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Comment on lines +137 to +141
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gross. Good work!


// 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