Foundation: State Components & Hooks #8931

JasonGore · 2019-05-02T22:41:17Z

Pull request checklist

Addresses an existing issue: Fixes Foundation: Finalize State & React Hooks #7014
Include a change request file using $ npm run change

Description of changes

This PR converts Foundation's state component type from classes to transform functions. This paves the way for these transform functions to use hooks.

Components that have state and use Foundation also needed to be converted as part of this change. In these cases their state components end up being one large hook used by Foundation.

In addition, some utility helpers have been added that can be used by components:

useControlledState: A React.useState wrapper that gives priority to controlled props.
getControlledDerivedProps: A simple helper that filters derived prop values while giving priority to controlled props.

Focus areas to test

Need to add hook unit tests as part of this PR.
Update create-component script.

Microsoft Reviewers: Open in CodeFlow

… have state components.

JasonGore · 2019-05-02T22:43:51Z

packages/experiments/src/components/Toggle/Toggle.state.ts

+  //         Return array from here including list of controlled props?
+  //         List of controlled props as createComponent option?
+  //         updateViewProps functional arg that takes in partial view props and optional controlled prop list?
+  viewProps.text = getControlledDerivedProps(viewProps, 'text', viewProps.checked ? viewProps.onText : viewProps.offText);


Lines 32-48 are lines I'm still noodling on. I'm not sure if additional helpers will hurt or hinder here.. but I also think it's important to provide tools that make sure people respect controlled props in a consistent and standardized way.

JasonGore · 2019-05-02T22:44:41Z

packages/experiments/src/components/PersonaCoin/PersonaCoin.state.ts

+export const usePersonaCoinState: IPersonaCoinComponent['state'] = props => {
+  // TODO: isPictureLoaded was controlled, does it need to be? it's not exposed through component props...
+  //       For now use useState.
+  const [isPictureLoaded, setIsPictureLoaded] = useState(false);


@Markionium Did you intend isPictureLoaded to be a controlled prop? Since it's not part of IPersonaCoinProps it couldn't actually be controlled without users generating a type error.

Yes that seems correct afaik.

Persona uses the Image component and we want to show the initials before showing you the picture. It might not make much sense to allow the user of the component determine some custom logic for how that works.

If we'll need to do that at some point in the future thats probably fine, but for now i think it's ok to keep this controlled.

The user can listen to updates in the loading state by using the onPhotoLoadingStateChange callback.

JasonGore · 2019-05-02T22:45:55Z

packages/foundation/src/createComponent.tsx

+      _defaultStyles: styles
+    } as TViewProps & IDefaultSlotProps<any>;
+
+    return component.view(viewProps);


I love how much simpler this is with the elimination of state class components and addition of hooks and useContext!

packages/experiments/src/components/PersonaCoin/examples/PersonaCoin.Example.tsx

Markionium

This is cool @JasonGore

Markionium · 2019-05-03T10:23:01Z

packages/experiments/src/components/PersonaCoin/PersonaCoin.state.ts

+export const usePersonaCoinState: IPersonaCoinComponent['state'] = props => {
+  // TODO: isPictureLoaded was controlled, does it need to be? it's not exposed through component props...
+  //       For now use useState.
+  const [isPictureLoaded, setIsPictureLoaded] = useState(false);


Yes that seems correct afaik.

Persona uses the Image component and we want to show the initials before showing you the picture. It might not make much sense to allow the user of the component determine some custom logic for how that works.

If we'll need to do that at some point in the future thats probably fine, but for now i think it's ok to keep this controlled.

The user can listen to updates in the loading state by using the onPhotoLoadingStateChange callback.

joschect · 2019-05-03T20:34:49Z

packages/experiments/src/components/Button/MenuButton/MenuButton.state.ts

+
+  const { disabled, onClick } = props;
+
+  const _onClick = useCallback(


One thing I wonder, would it be worthwhile to put this into their own state hook as well? Mainly to help set the precedent that we want generic helper functions that can act as building blocks.

I'd thought about it, but it seems behavior is so varied that the helpers would be so basic (or the options so complex) that I wonder if there is a net positive in having them. For example, do you always call callbacks? Do you call callbacks only if not processed? Do you stop propagation? Etc.

Like this would be a useExpandMenu hook that could be swapped around whenever we want a menu to get expanded.

I just feel like if we want there to be lots of shareable code between components, we should start trying now to set the standard that we want things to be in a common area. An example of where I would want this is for combobox, it should behave similarly IMO.

I agree and I've done that where I think it makes sense, like useControlledState. Here, though, we'd have to pass disabled, onClick, expanded, setExpanded, and setMenuTarget all to a helper. I don't think making this a helper is quite so black and white in this case. There is a balance between reuse and simplicity and I'm not sure making this common would add much value. The logic is highly customized to this component's use case while being relatively simple. Most of the implementation is primarily interaction with surface area (the inputs I mentioned above) which isn't meaningfully reduced by extracting it out as a separate function.

JasonGore · 2019-05-03T21:54:13Z

packages/experiments/src/components/Button/MenuButton/MenuButton.state.ts

+
+  const _onMenuDismiss = useCallback(() => {
+    setExpanded(false);
+  }, []);


General note due to some questions (and me forgetting temporarily): useCallback is used here to memoize the callback and prevent the callback's reference from changing on every render. React documentation states that setState functions are 'stable' and therefore do not need to appear in deps array. The effect of an empty array is that only one version of the callback will ever be memoized.

JasonGore · 2019-05-03T22:00:13Z

Ongoing investigation

The failed build state of this PR is due to multiple versions of react and react-dom being present across packages (16.8.6 in perf-test, and 16.8.2 in most of the other packages) which causes an Invalid Hook Warning in perf-test when it triggers a hook inside of OUFR (via Stack.)

Opened #8983 to fix the CI issue.

msft-github-bot · 2019-05-06T23:35:07Z

Component perf results:

Scenario	Target branch avg total (ms)	PR avg total (ms)	Target branch avg per item (ms)	PR avg per item (ms)	Is significant change	Is regression
PrimaryButton	90.652	94.336	0.907	0.943	false	false
BaseButton	36.603	37.733	0.366	0.377	false	false
NewButton	120.422	125.060	1.204	1.251	true	true
button	6.262	6.470	0.063	0.065	false	false
DetailsRows without styles	204.024	199.980	2.040	2.000	false	false
DetailsRows	226.335	226.141	2.263	2.261	false	false
Toggles	57.818	56.899	0.578	0.569	false	false
NewToggle	75.228	71.647	0.752	0.716	false	false
DocumentCardTitle with truncation	29.198	28.926	0.292	0.289	false	false

JasonGore · 2019-05-07T22:28:45Z

There's going to be more churn on this PR. I disabled a11y-tests to confirm that deployed sites are broken due to hardcoded React versions in index.html files. I'll fix all of the index.html files and confirm the deployed sites are working before re-enabling the a11y-tests as part of this PR.

dzearing · 2019-05-08T04:30:16Z

packages/experiments/src/components/Toggle/Toggle.state.ts

-    if (!disabled) {
-      // Only update the state if the user hasn't provided it.
-      this.setState({ checked: !checked });
+  const _onClick = useCallback(


still wrapping my brain on this... it seems that the function will be re-created on every render (but the useCallback will return the original value, I guess? for immutability purposes.)

I was wondering if we could use some kind of closure wrapper that encapsulates new function creation, passes in the dep values and only gets called when memoize retrieval fails. However, React states that there is no guarantee that previously memoized values will remain available, so we'd need some kind of hook to know when React is creating a new copy.

JasonGore added 2 commits May 2, 2019 15:32

Foundation: Convert state components to hooks and all components that…

21b1c42

… have state components.

Change files.

c4fb1a2

JasonGore added the Area: Foundation label May 2, 2019

JasonGore requested review from Markionium, dzearing, jdhuntington, joschect, khmakoto and micahgodbolt May 2, 2019 22:41

msft-github-bot added the Fabric 7 label May 2, 2019

msft-github-bot assigned apneer May 2, 2019

JasonGore commented May 2, 2019

View reviewed changes

JasonGore requested a review from kenotron May 2, 2019 22:56

joschect reviewed May 2, 2019

View reviewed changes

packages/experiments/src/components/PersonaCoin/examples/PersonaCoin.Example.tsx Outdated Show resolved Hide resolved

Minor fixes.

4f9eda3

Markionium approved these changes May 3, 2019

View reviewed changes

joschect reviewed May 3, 2019

View reviewed changes

JasonGore commented May 3, 2019

View reviewed changes

JasonGore assigned JasonGore and unassigned apneer May 3, 2019

This was referenced May 6, 2019

Button: Making all MenuButton props optional #8975

Merged

Reconcile React Dependency Versions #8983

Merged

JasonGore added 2 commits May 6, 2019 15:56

Merge branch 'fabric-7' into jg/foundation-hooks

1303bb7

Refresh change files from merge.

75d4285

JasonGore added 2 commits May 6, 2019 17:08

Foundation hook unit tests.

1cbb3b0

Merge branch 'fabric-7' into jg/foundation-hooks

710d16f

JasonGore added 3 commits May 7, 2019 08:14

Refresh shrinkwrap from merge.

7fcc28b

Update React dependencies.

d1022cf

Temporarily disable a11y tests for troubleshooting.

9fdf284

JasonGore requested review from a team, Raghurk and ecraig12345 as code owners May 7, 2019 20:07

JasonGore added the Status: Do Not Merge 🚫 label May 7, 2019

Update hardcoded React versions.

ff48350

JasonGore requested a review from jordandrako as a code owner May 7, 2019 23:14

Re-enable a11y-tests.

fd9925b

dzearing reviewed May 8, 2019

View reviewed changes

dzearing approved these changes May 8, 2019

View reviewed changes

dzearing merged commit 5f4051f into microsoft:fabric-7 May 8, 2019

This was referenced May 9, 2019

Create Component Script: Update for State & Hooks Approach #9025

Merged

Foundation: Finalize State & React Hooks #7014

Closed

Foundation: Evolve API #9072

Merged

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019


		const { disabled, onClick } = props;

		const _onClick = useCallback(

Foundation: State Components & Hooks #8931

Foundation: State Components & Hooks #8931

Uh oh!

Conversation

JasonGore commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Markionium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msft-github-bot commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonGore commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JasonGore commented May 2, 2019 •

edited

Loading

JasonGore May 2, 2019 •

edited

Loading

JasonGore May 3, 2019 •

edited

Loading

JasonGore May 3, 2019 •

edited

Loading

JasonGore commented May 3, 2019 •

edited

Loading

msft-github-bot commented May 6, 2019 •

edited

Loading