Skip to content

fix: Replaced custom style hooks defaultValue with proxy #27404

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.

Sign up for GitHub

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

Closed
GeoffCoxMSFT wants to merge 3 commits into from
Closed

fix: Replaced custom style hooks defaultValue with proxy #27404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76b2f33
Replaced defaultValue with smaller proxy
GeoffCoxMSFT Mar 31, 2023
75cf473
yarn change
GeoffCoxMSFT Mar 31, 2023
07e0d30
Merge branch 'master' into proxy-custom-style-hooks
GeoffCoxMSFT Mar 31, 2023
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
7 changes: 7 additions & 0 deletions change/@fluentui-react-shared-contexts-89b433e5-cfd8-427f-a5ea-89fbd4c51c3c.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Added proxy for default custom hook noop",
Copy link
Contributor

@ling1726 ling1726 Apr 3, 2023

Choose a reason for hiding this comment

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

Suggested change
"comment": "Added proxy for default custom hook noop",
"comment": "fix: Use proxy for default custom hook noop",

"packageName": "@fluentui/react-shared-contexts",
"email": "[email protected]",
"dependentChangeType": "patch"
}
96 changes: 12 additions & 84 deletions ...t-components/react-shared-contexts/src/CustomStyleHooksContext/CustomStyleHooksContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,92 +97,20 @@ export type CustomStyleHooksContextValue = {
export const CustomStyleHooksContext = React.createContext<CustomStyleHooksContextValue | undefined>(undefined);
Copy link
Collaborator

@fabricteam fabricteam Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🕵 fluentuiv9 Open the Visual Regressions report to inspect the 1 screenshots

✅ There was 0 screenshots added, 0 screenshots removed, 1925 screenshots unchanged, 0 screenshots with different dimensions and 1 screenshots with visible difference.

unknown 1 screenshots
Image Name Diff(in Pixels) Image Type
FluentProvider CustomStyleHooks.SplitButton.chromium.png 1605723 Changed


const noop = () => {};
const customStyleHooksContextDefaultValue: CustomStyleHooksContextValue = {
useAccordionHeaderStyles_unstable: noop,
useAccordionItemStyles_unstable: noop,
useAccordionPanelStyles_unstable: noop,
useAccordionStyles_unstable: noop,
useAvatarStyles_unstable: noop,
useAvatarGroupStyles_unstable: noop,
useAvatarGroupItemStyles_unstable: noop,
useAvatarGroupPopoverStyles_unstable: noop,
useBadgeStyles_unstable: noop,
useCounterBadgeStyles_unstable: noop,
useCardHeaderStyles_unstable: noop,
useCardStyles_unstable: noop,
useCardFooterStyles_unstable: noop,
useCardPreviewStyles_unstable: noop,
usePresenceBadgeStyles_unstable: noop,
useButtonStyles_unstable: noop,
useCompoundButtonStyles_unstable: noop,
useMenuButtonStyles_unstable: noop,
useSplitButtonStyles_unstable: noop,
useToggleButtonStyles_unstable: noop,
useCheckboxStyles_unstable: noop,
useComboboxStyles_unstable: noop,
useDropdownStyles_unstable: noop,
useListboxStyles_unstable: noop,
useOptionStyles_unstable: noop,
useOptionGroupStyles_unstable: noop,
useDividerStyles_unstable: noop,
useInputStyles_unstable: noop,
useImageStyles_unstable: noop,
useLabelStyles_unstable: noop,
useLinkStyles_unstable: noop,
useMenuDividerStyles_unstable: noop,
useMenuGroupHeaderStyles_unstable: noop,
useMenuGroupStyles_unstable: noop,
useMenuItemCheckboxStyles_unstable: noop,
useMenuItemRadioStyles_unstable: noop,
useMenuItemStyles_unstable: noop,
useMenuListStyles_unstable: noop,
useMenuPopoverStyles_unstable: noop,
useMenuSplitGroupStyles_unstable: noop,
usePersonaStyles_unstable: noop,
usePopoverSurfaceStyles_unstable: noop,
useRadioGroupStyles_unstable: noop,
useRadioStyles_unstable: noop,
useSelectStyles_unstable: noop,
useSliderStyles_unstable: noop,
useSpinButtonStyles_unstable: noop,
useSpinnerStyles_unstable: noop,
useSwitchStyles_unstable: noop,
useTabStyles_unstable: noop,
useTabListStyles_unstable: noop,
useTextStyles_unstable: noop,
useTextareaStyles_unstable: noop,
useTooltipStyles_unstable: noop,
useDialogTitleStyles_unstable: noop,
useDialogBodyStyles_unstable: noop,
useDialogActionsStyles_unstable: noop,
useDialogSurfaceStyles_unstable: noop,
useDialogContentStyles_unstable: noop,
useProgressBarStyles_unstable: noop,
useToolbarButtonStyles_unstable: noop,
useToolbarRadioButtonStyles_unstable: noop,
useToolbarGroupStyles_unstable: noop,
useToolbarToggleButtonStyles_unstable: noop,
useToolbarDividerStyles_unstable: noop,
useToolbarStyles_unstable: noop,
useTableCellStyles_unstable: noop,
useTableRowStyles_unstable: noop,
useTableBodyStyles_unstable: noop,
useTableStyles_unstable: noop,
useTableHeaderStyles_unstable: noop,
useTableHeaderCellStyles_unstable: noop,
useTableResizeHandleStyles_unstable: noop,
useTableSelectionCellStyles_unstable: noop,
useTableCellActionsStyles_unstable: noop,
useTableCellLayoutStyles_unstable: noop,
useDataGridCellStyles_unstable: noop,
useDataGridRowStyles_unstable: noop,
useDataGridBodyStyles_unstable: noop,
useDataGridStyles_unstable: noop,
useDataGridHeaderStyles_unstable: noop,
useDataGridHeaderCellStyles_unstable: noop,
useDataGridSelectionCellStyles_unstable: noop,

const preProxyDefaultValue: Partial<CustomStyleHooksContextValue> = {};

const defaultValueProxyHandler: ProxyHandler<CustomStyleHooksContextValue> = {
get(_target, _prop, _receiver) {
Copy link
Contributor

@ling1726 ling1726 Apr 3, 2023

Choose a reason for hiding this comment

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

Suggested change
get(_target, _prop, _receiver) {
get() {

return noop;
},
};

const customStyleHooksContextDefaultValue = new Proxy(
Copy link
Member

@layershifter layershifter Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@GeoffCoxMSFT I don't think that it works as expected as after a shallow merge in FluentProvider we are losing the proxy functionality. An example is below:

const noop = () => { };
const preProxyDefaultValue = {};

const defaultValueProxyHandler = {
    get() {
        return noop;
    },
};

const customStyleHooksContextDefaultValue = new Proxy(
  preProxyDefaultValue,
  defaultValueProxyHandler
);

// --- 

function shallowMerge(a, b) {
  // Merge impacts perf: we should like to avoid it if it's possible
  if (a && b) {
    return { ...a, ...b };
  }

  if (a) {
    return a;
  }

  return b;
}

// ---

const hooksFromProps = {
  useButton: 'hook',
  useImage: 'hook',
}
const result = shallowMerge(customStyleHooksContextDefaultValue, hooksFromProps)

⬇️⬇️⬇️

result.useButton // string 'hook' ✅

customStyleHooksContextDefaultValue.useFoo // function ✅ 
result.useFoo // undefined 🟥  We lost the proxy

Copy link
Member Author

@GeoffCoxMSFT GeoffCoxMSFT Apr 3, 2023

Choose a reason for hiding this comment

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

@layershifter Have you done much work with proxies in the past? I'm not sure what the fix would be here if spread breaks the proxy. Should we do object.assign instead?

Copy link
Member

@layershifter layershifter Apr 4, 2023

Choose a reason for hiding this comment

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

@GeoffCoxMSFT FYI: I just merged your PR with tests #27086, so we have a good base to test against 🎉

Option 1

There are few to be aware:

  • There is no isProxy() in JS, so we don't know with what we are dealing
  • Object spread will not work in in this case

To workaround that we can expose merge() function on the context:

// packages/react-components/react-shared-contexts/src/CustomStyleHooksContext/CustomStyleHooksContext.ts
  useDataGridHeaderCellStyles_unstable: CustomStyleHook;
  useDataGridSelectionCellStyles_unstable: CustomStyleHook;

+ merge: (overrides: CustomStyleHooksContextValue | Partial<CustomStyleHooksContextValue> | undefined) => void;
};

And then modify the handler:

const defaultValueProxyHandler: ProxyHandler<CustomStyleHooksContextValue> = {
  get(_target, _prop, _receiver) {
    if (_prop === 'merge') {
      return (overrides: CustomStyleHooksContextValue | Partial<CustomStyleHooksContextValue> | undefined) => {
        if (overrides) {
          Object.assign(preProxyDefaultValue, overrides);
        }
      };
    }

    if (_target[_prop]) {
      return _target[_prop];
    }

    return noop;
  },
};

(in this case we mutate target object + we are considering target as a source of hooks)

Then in useFluentProvider:

// packages/react-components/react-provider/src/components/FluentProvider/useFluentProvider.ts
-  // parentCustomStyleHooks will not be a partial
-  const mergedCustomStyleHooks = shallowMerge(
-    parentCustomStyleHooks,
-    customStyleHooks_unstable,
-  ) as CustomStyleHooksContextValue;
+. parentCustomStyleHooks.merge(customStyleHooks_unstable);

// ...
-  customStyleHooks_unstable: mergedCustomStyleHooks,
+  customStyleHooks_unstable: parentCustomStyleHooks,

Option 2

I talked with @ling1726 and his a better idea that does not require Proxy and IMO is clearer.

// The list of hooks is built from the exports from react-components/src/index
type ComponentNames = 'Button' | 'Image' // etc.

type CustomStyleHooksContextValue = {
  (componentName: ComponentNames): CustomStyleHook;
  current: Partial<Record<ComponentNames, CustomStyleHook>>;
};

/**
 * @internal
 */
export const CustomStyleHooksContext = React.createContext<CustomStyleHooksContextValue | undefined>(undefined);

const noop = () => {};

const customStyleHooksContextDefaultValue: CustomStyleHooksContextValue = componentName => {
  return customStyleHooksContextDefaultValue.current[componentName] || noop;
};

customStyleHooksContextDefaultValue.current = {};

In this case merging will be just:

const parentCustomStyleHooks = useCustomStyleHooks();

parentCustomStyleHooks.current = {
  ...parentCustomStyleHooks,
  ...props.customStyleHooks,
}

But it will require changes in components, for example:

-const { useButtonStyles_unstable: useCustomStyles } = useCustomStyleHooks_unstable();
-useCustomStyles(state);
+const useCustomStyles = useCustomStyleHooks_unstable();
+useCustomStyles('Button')(state)

I would say that this approach clearer, but it's up to you to decide 🐱

preProxyDefaultValue,
defaultValueProxyHandler,
) as CustomStyleHooksContextValue;

/**
* @internal
*/
Expand Down