fix: Replaced custom style hooks defaultValue with proxy #27404
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 07e0d30:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 34d618882a79b1cd65b2d4903f77caf9d5d83891 (build) |
Collaborator
📊 Bundle size report
Unchanged fixtures
|
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 881 | 907 | 5000 | |
| Button | mount | 590 | 583 | 5000 | |
| Field | mount | 1540 | 1544 | 5000 | |
| FluentProvider | mount | 1091 | 1079 | 5000 | |
| FluentProviderWithTheme | mount | 292 | 297 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 278 | 284 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 290 | 288 | 10 | |
| InfoButton | mount | 200 | 203 | 5000 | |
| MakeStyles | mount | 1407 | 1409 | 50000 | |
| Persona | mount | 2126 | 2103 | 5000 | |
| SpinButton | mount | 1833 | 1851 | 5000 |
fabricteam
reviewed
Mar 31, 2023
| /** | ||
| * @internal | ||
| */ | ||
| export const CustomStyleHooksContext = React.createContext<CustomStyleHooksContextValue | undefined>(undefined); |
Collaborator
There was a problem hiding this comment.
🕵 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 |
ling1726
reviewed
Apr 3, 2023
| const preProxyDefaultValue: Partial<CustomStyleHooksContextValue> = {}; | ||
|
|
||
| const defaultValueProxyHandler: ProxyHandler<CustomStyleHooksContextValue> = { | ||
| get(_target, _prop, _receiver) { |
Contributor
There was a problem hiding this comment.
Suggested change
| get(_target, _prop, _receiver) { | |
| get() { |
ling1726
reviewed
Apr 3, 2023
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "type": "patch", | |||
| "comment": "Added proxy for default custom hook noop", | |||
Contributor
There was a problem hiding this comment.
Suggested change
| "comment": "Added proxy for default custom hook noop", | |
| "comment": "fix: Use proxy for default custom hook noop", |
ling1726
previously approved these changes
Apr 3, 2023
layershifter
requested changes
Apr 3, 2023
| }, | ||
| }; | ||
|
|
||
| const customStyleHooksContextDefaultValue = new Proxy( |
Member
There was a problem hiding this comment.
@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
Member
Author
There was a problem hiding this comment.
@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?
Member
There was a problem hiding this comment.
@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 🐱
msft-fluent-ui-bot
added
Needs: Author Feedback
and removed
Needs: Author Feedback
labels
Member
Author
|
Closing in favor of #27491 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The large number of no-op implementation for the default value of the custom hooks is preventing bundling minification.
Changes
Issues
Fixes #27139