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

[embeddable rebuild] useBatchedOptionalPublishingSubjects #180221

Merged
merged 18 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 17 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
1 change: 1 addition & 0 deletions packages/presentation/presentation_publishing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export {
} from './interfaces/titles/publishes_panel_title';
export { initializeTitles, type SerializedTitles } from './interfaces/titles/titles_api';
export {
useBatchedOptionalPublishingSubjects,
useBatchedPublishingSubjects,
usePublishingSubject,
useStateFromPublishingSubject,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
* Side Public License, v 1.
*/

export { useBatchedPublishingSubjects } from './publishing_batcher';
export {
useBatchedOptionalPublishingSubjects,
useBatchedPublishingSubjects,
} from './publishing_batcher';
export { useStateFromPublishingSubject, usePublishingSubject } from './publishing_subject';
export type {
PublishingSubject,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { useEffect, useMemo, useRef, useState } from 'react';
import { useEffect, useRef, useState } from 'react';
import { combineLatest, debounceTime, skip } from 'rxjs';
import { AnyPublishingSubject, PublishingSubject, UnwrapPublishingSubjectTuple } from './types';

Expand All @@ -25,25 +25,28 @@ const hasSubjectsArrayChanged = (

/**
* Batches the latest values of multiple publishing subjects into a single object. Use this to avoid unnecessary re-renders.
* You should avoid using this hook with subjects that your component pushes values to on user interaction, as it can cause a slight delay.
* Use when `subjects` may not be defined on initial component render.
*
* @param subjects Publishing subjects array.
* When 'subjects' is expected to change, 'subjects' must be part of component react state.
*/
export const useBatchedPublishingSubjects = <SubjectsType extends [...AnyPublishingSubject[]]>(
export const useBatchedOptionalPublishingSubjects = <
SubjectsType extends [...AnyPublishingSubject[]]
>(
...subjects: [...SubjectsType]
): UnwrapPublishingSubjectTuple<SubjectsType> => {
const isFirstRender = useRef(true);
/**
* memoize and deep diff subjects to avoid rebuilding the subscription when the subjects are the same.
*/

const previousSubjects = useRef<SubjectsType>(subjects);
const subjectsToUse = useMemo(() => {
// Can not use 'useMemo' because 'subjects' gets a new reference on each call because of spread
const subjectsToUse = (() => {
// avoid rebuilding the subscription when the subjects are the same
if (!hasSubjectsArrayChanged(previousSubjects.current ?? [], subjects)) {
return previousSubjects.current;
}
previousSubjects.current = subjects;
return subjects;
}, [subjects]);
})();

/**
* Set up latest published values state, initialized with the current values of the subjects.
Expand Down Expand Up @@ -94,6 +97,46 @@ export const useBatchedPublishingSubjects = <SubjectsType extends [...AnyPublish
return latestPublishedValues;
};

/**
* Batches the latest values of multiple publishing subjects into a single object. Use this to avoid unnecessary re-renders.
nreese marked this conversation as resolved.
Show resolved Hide resolved
* Use when `subjects` are static and do not change over the lifetime of the component.
*
* @param subjects Publishing subjects array.
*/
export const useBatchedPublishingSubjects = <
SubjectsType extends [...Array<PublishingSubject<any>>]
>(
...subjects: [...SubjectsType]
): UnwrapPublishingSubjectTuple<SubjectsType> => {
/**
* Set up latest published values state, initialized with the current values of the subjects.
*/
const [latestPublishedValues, setLatestPublishedValues] = useState<
nreese marked this conversation as resolved.
Show resolved Hide resolved
UnwrapPublishingSubjectTuple<SubjectsType>
>(() => unwrapPublishingSubjectArray(subjects));

/**
* Subscribe to all subjects and update the latest values when any of them change.
*/
useEffect(() => {
const subscription = combineLatest(subjects)
.pipe(
// When a new observer subscribes to a BehaviorSubject, it immediately receives the current value. Skip this emit.
skip(1),
debounceTime(0)
)
.subscribe((values) => {
setLatestPublishedValues(values as UnwrapPublishingSubjectTuple<SubjectsType>);
});
return () => subscription.unsubscribe();
// 'subjects' gets a new reference on each call because of spread
// Use 'useBatchedOptionalPublishingSubjects' when 'subjects' are expected to change.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return latestPublishedValues;
};

const unwrapPublishingSubjectArray = <T extends AnyPublishingSubject[]>(
subjects: T
): UnwrapPublishingSubjectTuple<T> => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@ import { BehaviorSubject } from 'rxjs';
import { render, screen, waitFor } from '@testing-library/react';
import '@testing-library/jest-dom';
import userEvent from '@testing-library/user-event';
import { useBatchedPublishingSubjects } from './publishing_batcher';
import {
useBatchedPublishingSubjects,
useBatchedOptionalPublishingSubjects,
} from './publishing_batcher';
import { useStateFromPublishingSubject } from './publishing_subject';
import { PublishingSubject } from './types';

describe('useBatchedPublishingSubjects', () => {
describe('publishing subject', () => {
describe('render', () => {
let subject1: BehaviorSubject<number>;
let subject2: BehaviorSubject<number>;
Expand Down Expand Up @@ -56,7 +59,6 @@ describe('useBatchedPublishingSubjects', () => {
<>
<button onClick={incrementAll} />
<span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
<div data-test-subj="renderCount">{renderCount}</div>
</>
);
}
Expand All @@ -72,7 +74,7 @@ describe('useBatchedPublishingSubjects', () => {
screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
).toBeInTheDocument();
});
expect(screen.getByTestId('renderCount')).toHaveTextContent('2');
expect(renderCount).toBe(2);
});

test('should batch state updates when using useBatchedPublishingSubjects', async () => {
Expand All @@ -97,7 +99,6 @@ describe('useBatchedPublishingSubjects', () => {
}}
/>
<span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
<div data-test-subj="renderCount">{renderCount}</div>
</>
);
}
Expand All @@ -113,7 +114,48 @@ describe('useBatchedPublishingSubjects', () => {
screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
).toBeInTheDocument();
});
expect(screen.getByTestId('renderCount')).toHaveTextContent('2');
expect(renderCount).toBe(2);
});

test('should batch state updates when using useBatchedOptionalPublishingSubjects', async () => {
let renderCount = 0;
function Component() {
const [value1, value2, value3, value4, value5, value6] =
useBatchedOptionalPublishingSubjects(
subject1,
subject2,
subject3,
subject4,
subject5,
subject6
);

renderCount++;
return (
<>
<button
onClick={() => {
// using setTimeout to move next calls outside of callstack from onClick
setTimeout(incrementAll, 0);
}}
/>
<span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
</>
);
}
render(<Component />);
await waitFor(() => {
expect(
screen.getByText('value1: 0, value2: 0, value3: 0, value4: 0, value5: 0, value6: 0')
).toBeInTheDocument();
});
userEvent.click(screen.getByRole('button'));
await waitFor(() => {
expect(
screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
).toBeInTheDocument();
});
expect(renderCount).toBe(2);
});

test('should render for each state update outside of click handler', async () => {
Expand All @@ -136,7 +178,6 @@ describe('useBatchedPublishingSubjects', () => {
}}
/>
<span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
<div data-test-subj="renderCount">{renderCount}</div>
</>
);
}
Expand All @@ -152,33 +193,33 @@ describe('useBatchedPublishingSubjects', () => {
screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
).toBeInTheDocument();
});
expect(screen.getByTestId('renderCount')).toHaveTextContent('7');
expect(renderCount).toBe(7);
});
});

describe('Publishing subject is undefined on first render', () => {
test('useBatchedPublishingSubjects state should update when publishing subject is provided', async () => {
test('useBatchedOptionalPublishingSubjects should update state when publishing subject is provided', async () => {
let renderCount = 0;
function Component() {
// When subjects is expected to change, subjects must be part of react state.
const [subjectFoo, setSubjectFoo] = useState<PublishingSubject<string> | undefined>(
const [subjectFoo, setSubjectFoo] = useState<BehaviorSubject<string> | undefined>(
undefined
);
const [valueFoo] = useBatchedPublishingSubjects(subjectFoo);
const [valueFoo] = useBatchedOptionalPublishingSubjects(subjectFoo);

renderCount++;
return (
<>
<button
onClick={() => {
// using setTimeout to move next calls outside of callstack from onClick
setTimeout(() => {
setSubjectFoo(new BehaviorSubject<string>('foo'));
}, 0);
if (!subjectFoo) {
setSubjectFoo(new BehaviorSubject<string>('foo1'));
} else {
subjectFoo.next('foo2');
}
}}
/>
<span>{`valueFoo: ${valueFoo}`}</span>
<div data-test-subj="renderCount">{renderCount}</div>
</>
);
}
Expand All @@ -187,13 +228,14 @@ describe('useBatchedPublishingSubjects', () => {
expect(screen.getByText('valueFoo: undefined')).toBeInTheDocument();
});
userEvent.click(screen.getByRole('button'));
userEvent.click(screen.getByRole('button'));
await waitFor(() => {
expect(screen.getByText('valueFoo: foo')).toBeInTheDocument();
expect(screen.getByText('valueFoo: foo2')).toBeInTheDocument();
});
expect(screen.getByTestId('renderCount')).toHaveTextContent('3');
expect(renderCount).toBe(4);
});

test('useStateFromPublishingSubject state should update when publishing subject is provided', async () => {
test('useStateFromPublishingSubject should update state when publishing subject is provided', async () => {
let renderCount = 0;
function Component() {
// When subject is expected to change, subject must be part of react state.
Expand All @@ -207,14 +249,10 @@ describe('useBatchedPublishingSubjects', () => {
<>
<button
onClick={() => {
// using setTimeout to move next calls outside of callstack from onClick
setTimeout(() => {
setSubjectFoo(new BehaviorSubject<string>('foo'));
}, 0);
setSubjectFoo(new BehaviorSubject<string>('foo'));
}}
/>
<span>{`valueFoo: ${valueFoo}`}</span>
<div data-test-subj="renderCount">{renderCount}</div>
</>
);
}
Expand All @@ -226,7 +264,7 @@ describe('useBatchedPublishingSubjects', () => {
await waitFor(() => {
expect(screen.getByText('valueFoo: foo')).toBeInTheDocument();
});
expect(screen.getByTestId('renderCount')).toHaveTextContent('3');
expect(renderCount).toBe(3);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { ImageViewer } from './image_viewer';
import { ImageViewerContext } from './image_viewer/image_viewer_context';

import './image_embeddable.scss';
import { BehaviorSubject } from 'rxjs';

interface ImageEmbeddableProps {
api: ImageEmbeddableApi & {
Expand All @@ -32,7 +33,7 @@ interface ImageEmbeddableProps {
export const ImageEmbeddable = ({ api, filesClient }: ImageEmbeddableProps) => {
const [imageConfig, dynamicActionsState] = useBatchedPublishingSubjects(
api.imageConfig$,
api.dynamicActionsState$
api.dynamicActionsState$ ?? new BehaviorSubject<undefined>(undefined)
);
const [hasTriggerActions, setHasTriggerActions] = useState(false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import {
} from '@elastic/eui';
import { Action, buildContextMenuForActions } from '@kbn/ui-actions-plugin/public';

import { getViewModeSubject, useBatchedPublishingSubjects } from '@kbn/presentation-publishing';
import {
getViewModeSubject,
useBatchedOptionalPublishingSubjects,
} from '@kbn/presentation-publishing';
import { uiActions } from '../../kibana_services';
import { contextMenuTrigger, CONTEXT_MENU_TRIGGER } from '../../panel_actions';
import { getContextMenuAriaLabel } from '../presentation_panel_strings';
Expand All @@ -43,7 +46,7 @@ export const PresentationPanelContextMenu = ({
const [isContextMenuOpen, setIsContextMenuOpen] = useState<boolean | undefined>(undefined);
const [contextMenuPanels, setContextMenuPanels] = useState<EuiContextMenuPanelDescriptor[]>([]);

const [title, parentViewMode] = useBatchedPublishingSubjects(
const [title, parentViewMode] = useBatchedOptionalPublishingSubjects(
api.panelTitle,

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
apiPublishesPhaseEvents,
apiHasParentApi,
apiPublishesViewMode,
useBatchedPublishingSubjects,
useBatchedOptionalPublishingSubjects,
} from '@kbn/presentation-publishing';
import classNames from 'classnames';
import React, { useEffect, useMemo, useState } from 'react';
Expand Down Expand Up @@ -57,7 +57,7 @@ export const PresentationPanelInternal = <
defaultPanelTitle,
rawViewMode,
parentHidePanelTitle,
] = useBatchedPublishingSubjects(
] = useBatchedOptionalPublishingSubjects(
api?.dataLoading,
api?.blockingError,
api?.panelTitle,
Expand Down