-
Notifications
You must be signed in to change notification settings - Fork 33
feat: DH-14581 usePanelRegistration hook #1208
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /* eslint-disable max-classes-per-file */ | ||
| import React from 'react'; | ||
| import { renderHook } from '@testing-library/react-hooks'; | ||
| import usePanelRegistration from './usePanelRegistration'; | ||
| import { PanelProps } from '../DashboardPlugin'; | ||
|
|
||
| /* eslint-disable react/prefer-stateless-function */ | ||
| class ClassCOMPONENT extends React.Component<PanelProps> { | ||
| static COMPONENT = 'ClassCOMPONENT'; | ||
| } | ||
| class ClassDisplayName extends React.Component<PanelProps> { | ||
| static displayName = 'ClassDisplayName'; | ||
| } | ||
| class ClassNoName extends React.Component<PanelProps> {} | ||
| /* eslint-enable react/prefer-stateless-function */ | ||
|
|
||
| function FnCOMPONENT() { | ||
| return null; | ||
| } | ||
| FnCOMPONENT.COMPONENT = 'FnCOMPONENT'; | ||
| function FnDisplayName() { | ||
| return null; | ||
| } | ||
| FnDisplayName.displayName = 'FnDisplayName'; | ||
| function FnNoName() { | ||
| return null; | ||
| } | ||
|
|
||
| const deregister = jest.fn(); | ||
| const registerComponent = jest.fn(); | ||
| const hydrate = jest.fn(); | ||
| const dehydrate = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| registerComponent.mockReturnValue(deregister); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [ClassCOMPONENT.COMPONENT, ClassCOMPONENT], | ||
| [ClassDisplayName.displayName, ClassDisplayName], | ||
| [FnCOMPONENT.COMPONENT, FnCOMPONENT], | ||
| [FnDisplayName.displayName, FnDisplayName], | ||
| ])( | ||
| 'should register components with COMPONENT or displayName attributes and deregister on unmount: "%s"', | ||
| (_label, ComponentType) => { | ||
| const { unmount } = renderHook(() => | ||
| usePanelRegistration(registerComponent, ComponentType, hydrate, dehydrate) | ||
| ); | ||
|
|
||
| const name = | ||
| 'COMPONENT' in ComponentType | ||
| ? ComponentType.COMPONENT | ||
| : ComponentType.displayName; | ||
|
|
||
| expect(registerComponent).toHaveBeenCalledWith( | ||
| name, | ||
| ComponentType, | ||
| hydrate, | ||
| dehydrate | ||
| ); | ||
|
|
||
| expect(deregister).not.toHaveBeenCalled(); | ||
|
|
||
| unmount(); | ||
|
|
||
| expect(deregister).toHaveBeenCalled(); | ||
| } | ||
| ); | ||
|
|
||
| it.each([ | ||
| ['MockClassNoName', ClassNoName], | ||
| ['MockFnNoName', FnNoName], | ||
| ])( | ||
| 'should throw an error if no COMPONENT or displayName attribute exists: "%s"', | ||
| (_label, ComponentType) => { | ||
| const { result } = renderHook(() => | ||
| usePanelRegistration(registerComponent, ComponentType, hydrate, dehydrate) | ||
| ); | ||
|
|
||
| expect(result.error).toEqual( | ||
| new Error( | ||
| 'ComponentType must have a `COMPONENT` or `displayName` attribute.' | ||
| ) | ||
| ); | ||
| } | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import React from 'react'; | ||
| import { | ||
| DashboardPluginComponentProps, | ||
| PanelComponentType, | ||
| PanelDehydrateFunction, | ||
| PanelHydrateFunction, | ||
| PanelProps, | ||
| } from '../DashboardPlugin'; | ||
|
|
||
| /** | ||
| * Registers a given panel component. Also runs a `useEffect` that will | ||
| * automatically de-register then panel on unmount. | ||
| * @param registerComponent | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might not want this as part of the hook. Bender and I chatted about plugins the other day and there's a lot of rough edges and things that need to be cleaned up around their implementation. Basically we should be able to write our existing plugins all as external plugins in an ideal world hydrate and dehydrate are the same. IMO they are logic that belong in the component, not at the registration point. Our existing hydration code is in AppMainContainer which separates it from where the hydration is actually used. I don't think they're changes that should be in this PR, but just something to note that this will probably change signatures as we address those points |
||
| * @param ComponentType | ||
| * @param hydrate | ||
| * @param dehydrate | ||
| */ | ||
| export default function usePanelRegistration< | ||
| P extends PanelProps, | ||
| C extends React.ComponentType<P> | ||
| >( | ||
| registerComponent: DashboardPluginComponentProps['registerComponent'], | ||
| ComponentType: PanelComponentType<P, C>, | ||
| hydrate?: PanelHydrateFunction<P>, | ||
| dehydrate?: PanelDehydrateFunction | ||
| ) { | ||
| const name = ComponentType.COMPONENT ?? ComponentType.displayName; | ||
|
|
||
| if (name == null) { | ||
| throw new Error( | ||
| 'ComponentType must have a `COMPONENT` or `displayName` attribute.' | ||
| ); | ||
| } | ||
|
|
||
| React.useEffect(() => { | ||
| const deregister = registerComponent( | ||
| name, | ||
| ComponentType, | ||
| hydrate, | ||
| dehydrate | ||
| ); | ||
|
|
||
| return () => { | ||
| deregister(); | ||
| }; | ||
| }, [ComponentType, dehydrate, hydrate, name, registerComponent]); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is used here and in
PanelComponentType, may want to separate it out and add some JSDocs about what each is for