From 476dd0d2c8b237606b956dee738f9d9b3c56f6db Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Wed, 23 Aug 2023 08:59:27 +0000 Subject: [PATCH] bugfix: ensure interop between assertSlots and old API --- ...-75019ddc-695b-4255-9b47-0aa69be7fbcf.json | 7 + .../react-jsx-runtime/src/interop.test.tsx | 190 ++++++++++++++++++ .../src/compose/assertSlots.test.tsx | 17 +- .../src/compose/assertSlots.ts | 21 +- 4 files changed, 227 insertions(+), 8 deletions(-) create mode 100644 change/@fluentui-react-utilities-75019ddc-695b-4255-9b47-0aa69be7fbcf.json create mode 100644 packages/react-components/react-jsx-runtime/src/interop.test.tsx diff --git a/change/@fluentui-react-utilities-75019ddc-695b-4255-9b47-0aa69be7fbcf.json b/change/@fluentui-react-utilities-75019ddc-695b-4255-9b47-0aa69be7fbcf.json new file mode 100644 index 00000000000000..08ded8898f87a6 --- /dev/null +++ b/change/@fluentui-react-utilities-75019ddc-695b-4255-9b47-0aa69be7fbcf.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "bugfix: ensure interop between assertSlots and old API", + "packageName": "@fluentui/react-utilities", + "email": "bernardo.sunderhus@gmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-jsx-runtime/src/interop.test.tsx b/packages/react-components/react-jsx-runtime/src/interop.test.tsx new file mode 100644 index 00000000000000..2824f03f7ac87d --- /dev/null +++ b/packages/react-components/react-jsx-runtime/src/interop.test.tsx @@ -0,0 +1,190 @@ +/** @jsxRuntime classic */ +/** @jsx createElement */ + +import { render } from '@testing-library/react'; +import { assertSlots, resolveShorthand, slot } from '@fluentui/react-utilities'; +import type { ComponentProps, ComponentState, Slot } from '@fluentui/react-utilities'; +import { createElement } from './createElement'; + +describe('resolveShorthand with assertSlots', () => { + describe('custom behavior tests', () => { + it('keeps children from "defaultProps" in a render callback', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); + type TestComponentSlots = { + someSlot: NonNullable>; + }; + type TestComponentProps = ComponentProps>; + type TestComponentState = ComponentState; + + const TestComponent = (props: TestComponentProps) => { + const state: TestComponentState = { + components: { someSlot: 'div' }, + someSlot: resolveShorthand(props.someSlot, { + required: true, + defaultProps: { children: 'Default Children', id: 'slot' }, + }), + }; + assertSlots(state); + return ; + }; + + const children = jest.fn().mockImplementation((Component, props) => ( +
+ +
+ )); + const result = render(); + + expect(children).toHaveBeenCalledTimes(1); + expect(children).toHaveBeenCalledWith('div', { children: 'Default Children', id: 'slot' }); + + expect(result.container.firstChild).toMatchInlineSnapshot(` +
+
+ Default Children +
+
+ `); + + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); + }); + + it('keeps children from a render template in a render callback', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); + type TestComponentSlots = { outer: NonNullable>; inner: NonNullable> }; + type TestComponentState = ComponentState; + type TestComponentProps = ComponentProps>; + + const TestComponent = (props: TestComponentProps) => { + const state: TestComponentState = { + components: { outer: 'div', inner: 'div' }, + inner: resolveShorthand(props.inner, { defaultProps: { id: 'inner' }, required: true }), + outer: resolveShorthand(props.outer, { defaultProps: { id: 'outer' }, required: true }), + }; + assertSlots(state); + return ( + + + + ); + }; + + const children = jest.fn().mockImplementation((Component, props) => ( +
+ +
+ )); + const result = render(); + + expect(children).toHaveBeenCalledTimes(1); + expect(children.mock.calls[0][0]).toBe('div'); + expect(children.mock.calls[0][1].id).toBe('outer'); + expect(children.mock.calls[0][1].children).toMatchInlineSnapshot(` + +
+ Inner children +
+ + `); + + expect(result.container.firstChild).toMatchInlineSnapshot(` +
+
+
+ Inner children +
+
+
+ `); + expect(consoleWarnMock).toHaveBeenCalledTimes(2); + consoleWarnMock.mockRestore(); + }); + + it("should support 'as' property to opt-out of base element type", () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); + type TestComponentSlots = { slot: NonNullable> }; + type TestComponentState = ComponentState; + type TestComponentProps = ComponentProps>; + + const TestComponent = (props: TestComponentProps) => { + const state: TestComponentState = { + components: { slot: 'div' }, + slot: resolveShorthand(props.slot, { required: true }), + }; + assertSlots(state); + return ; + }; + + const result = render(); + + expect(result.container.firstChild).toMatchInlineSnapshot(``); + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); + }); + it('should support if someone passes an invalid slot', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); + type TestComponentSlots = { slot: NonNullable> }; + type TestComponentState = ComponentState; + type TestComponentProps = ComponentProps>; + + const TestComponent = (props: TestComponentProps) => { + const state: TestComponentState = { + components: { slot: 'div' }, + slot: {}, + }; + assertSlots(state); + return ; + }; + + const result = render(); + + expect(result.container.firstChild).toMatchInlineSnapshot(`
`); + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); + }); + it('should support overriding a slot declaration with spread props', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); + type TestComponentSlots = { slot: NonNullable> }; + type TestComponentState = ComponentState; + type TestComponentProps = ComponentProps>; + const useHigherOrderStateHook = (props: TestComponentProps): TestComponentState => ({ + components: { slot: 'div' }, + slot: slot.always(props.slot, { elementType: 'div' }), + }); + const TestComponent = (props: TestComponentProps) => { + const higherOrderState = useHigherOrderStateHook(props); + const state: TestComponentState = { + components: { ...higherOrderState.components, slot: 'span' }, + slot: { + ...higherOrderState.slot, + children: 'slot children', + }, + }; + assertSlots(state); + return ; + }; + const result = render(); + + expect(result.container.firstChild).toMatchInlineSnapshot(` + + slot children + + `); + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); + }); + }); +}); diff --git a/packages/react-components/react-utilities/src/compose/assertSlots.test.tsx b/packages/react-components/react-utilities/src/compose/assertSlots.test.tsx index b22cbdecf3cd8f..85295b0dbc3f21 100644 --- a/packages/react-components/react-utilities/src/compose/assertSlots.test.tsx +++ b/packages/react-components/react-utilities/src/compose/assertSlots.test.tsx @@ -1,4 +1,5 @@ import { assertSlots } from './assertSlots'; +import { SLOT_ELEMENT_TYPE_SYMBOL } from './constants'; import * as slot from './slot'; import { ComponentProps, ComponentState, Slot } from './types'; @@ -27,7 +28,8 @@ describe('assertSlots', () => { }; expect(() => assertSlots(state)).not.toThrow(); }); - it('should throw if a slot is not declared with the `slot` function', () => { + it('should not throw if a slot is not declared with the `slot` function', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); const state: TestState = { components: { slotA: 'div', @@ -36,9 +38,13 @@ describe('assertSlots', () => { }, slotA: {}, }; - expect(() => assertSlots(state)).toThrow(); + expect(() => assertSlots(state)).not.toThrow(); + expect(state.slotA).toEqual({ [SLOT_ELEMENT_TYPE_SYMBOL]: 'div' }); + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); }); - it('should throw if a state.components.SLOT_NAME is not equivalent to the slot elementType', () => { + it('should not throw if a state.components.SLOT_NAME is not equivalent to the slot elementType', () => { + const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation(); const props: TestProps = { slotA: 'hello' }; const state: TestState = { components: { @@ -48,6 +54,9 @@ describe('assertSlots', () => { }, slotA: slot.optional(props.slotA, { elementType: 'div' }), }; - expect(() => assertSlots(state)).toThrow(); + expect(() => assertSlots(state)).not.toThrow(); + expect(state.slotA).toEqual({ children: 'hello', [SLOT_ELEMENT_TYPE_SYMBOL]: 'a' }); + expect(consoleWarnMock).toHaveBeenCalledTimes(1); + consoleWarnMock.mockRestore(); }); }); diff --git a/packages/react-components/react-utilities/src/compose/assertSlots.ts b/packages/react-components/react-utilities/src/compose/assertSlots.ts index 8280b51bba4b4a..d5f9284b9ae5fc 100644 --- a/packages/react-components/react-utilities/src/compose/assertSlots.ts +++ b/packages/react-components/react-utilities/src/compose/assertSlots.ts @@ -1,6 +1,8 @@ +import * as React from 'react'; import { SLOT_ELEMENT_TYPE_SYMBOL } from './constants'; import { isSlot } from './isSlot'; import { ComponentState, ExtractSlotProps, SlotComponentType, SlotPropsRecord } from './types'; +import { slot } from './index'; type SlotComponents = { [K in keyof Slots]: SlotComponentType>; @@ -37,16 +39,27 @@ export function assertSlots(state: unknown): asse if (slotElement === undefined) { continue; } + // this means a slot is being declared without using, slot.always or slot.optional or even resolveShorthand on the state hook, + // but the render method is using the new `assertSlots` method. That scenario can be solved by simply updating the slot element with the proper element type + // FIXME: this slot will still fail to support child render function scenario if (!isSlot(slotElement)) { - throw new Error( - `${assertSlots.name} error: state.${slotName} is not a slot.\n` + + typedState[slotName as keyof ComponentState] = slot.always(slotElement, { + elementType: typedState.components[slotName] as React.ComponentType<{}>, + }) as ComponentState[keyof ComponentState]; + // eslint-disable-next-line no-console + console.warn( + `${assertSlots.name} warning: state.${slotName} is not a slot.\n` + `Be sure to create slots properly by using 'slot.always' or 'slot.optional'.`, ); } else { + // This means a slot is being declared by using resolveShorthand on the state hook, + // but the render method is using the new `assertSlots` method. That scenario can be solved by simply updating the slot element with the proper element type const { [SLOT_ELEMENT_TYPE_SYMBOL]: elementType } = slotElement; if (elementType !== typedState.components[slotName]) { - throw new TypeError( - `${assertSlots.name} error: state.${slotName} element type differs from state.components.${slotName}, ${elementType} !== ${typedState.components[slotName]}. \n` + + slotElement[SLOT_ELEMENT_TYPE_SYMBOL] = typedState.components[slotName] as React.ComponentType<{}>; + // eslint-disable-next-line no-console + console.warn( + `${assertSlots.name} warning: state.${slotName} element type differs from state.components.${slotName}, ${elementType} !== ${typedState.components[slotName]}. \n` + `Be sure to create slots properly by using 'slot.always' or 'slot.optional' with the correct elementType`, ); }