Skip to content
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

Enable eslint-plugin-react-hooks for react-next #14478

Merged
merged 6 commits into from
Aug 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Original file line Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "patch",
"comment": "Add rule to prevent accidental references to globals",
"packageName": "@fluentui/eslint-plugin",
"email": "[email protected]",
"dependentChangeType": "patch",
"date": "2020-08-12T01:34:12.441Z"
}
Original file line number Original file line Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "prerelease",
"comment": "Enable eslint-plugin-react-hooks in react-next and fix a few bugs",
"packageName": "@fluentui/react-next",
"email": "[email protected]",
"dependentChangeType": "patch",
"date": "2020-08-12T01:34:16.311Z"
}
Original file line number Original file line Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "none",
"comment": "Fix violation of newly added lint rule",
"packageName": "@uifabric/monaco-editor",
"email": "[email protected]",
"dependentChangeType": "none",
"date": "2020-08-12T02:02:22.263Z"
}
Original file line number Original file line Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "none",
"comment": "Fix misleading docs for safeMount",
"packageName": "@uifabric/test-utilities",
"email": "[email protected]",
"dependentChangeType": "none",
"date": "2020-08-12T01:34:27.992Z"
}
8 changes: 7 additions & 1 deletion packages/eslint-plugin/src/configs/react.js
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ const config = {
'no-empty': 'error', 'no-empty': 'error',
'no-eval': 'error', 'no-eval': 'error',
'no-new-wrappers': 'error', 'no-new-wrappers': 'error',
'no-restricted-globals': [
'error',
...['blur', 'close', 'focus', 'length', 'name', 'parent', 'self', 'stop'].map(name => ({
name,
message: `"${name}" refers to a DOM global. Did you mean to reference a local value instead?`,
})),
],
'no-restricted-properties': [ 'no-restricted-properties': [
'error', 'error',
{ object: 'describe', property: 'only', message: 'describe.only should only be used during test development' }, { object: 'describe', property: 'only', message: 'describe.only should only be used during test development' },
Expand Down Expand Up @@ -204,7 +211,6 @@ const config = {
// permanently disable because we disagree with these rules // permanently disable because we disagree with these rules
'import/prefer-default-export': 'off', 'import/prefer-default-export': 'off',
'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable 'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable
'no-restricted-globals': 'off', // airbnb bans isNaN in favor of Number.isNaN which is unavailable in IE 11
'react/jsx-props-no-spreading': 'off', 'react/jsx-props-no-spreading': 'off',
'react/prop-types': 'off', 'react/prop-types': 'off',


Expand Down
1 change: 1 addition & 0 deletions packages/monaco-editor/src/configureEnvironment.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export interface IMonacoConfig {
crossDomain?: boolean; crossDomain?: boolean;
} }


// eslint-disable-next-line no-restricted-globals
const globalObj = (typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {}) as Window & { const globalObj = (typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {}) as Window & {
// eslint-disable-next-line @typescript-eslint/no-explicit-any // eslint-disable-next-line @typescript-eslint/no-explicit-any
MonacoEnvironment?: any; MonacoEnvironment?: any;
Expand Down
5 changes: 1 addition & 4 deletions packages/react-next/.eslintrc.json
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
"extends": ["../office-ui-fabric-react/.eslintrc"], "extends": ["../office-ui-fabric-react/.eslintrc"],
"root": true, "root": true,
"rules": { "rules": {
"@typescript-eslint/no-explicit-any": "error", "@typescript-eslint/no-explicit-any": "error"
// Disable until issues are dealt with
"react-hooks/exhaustive-deps": "off",
"react-hooks/rules-of-hooks": "off"
} }
} }
31 changes: 20 additions & 11 deletions packages/react-next/RELEASE_NOTES.md
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -2,17 +2,9 @@


## Breaking changes ## Breaking changes


### Beak ### Calendar


- Removed empty `IBeak` interface TODO: Diff of OUFR vs date-time Calendar
ecraig12345 marked this conversation as resolved.
Show resolved Hide resolved
- Removed `componentRef` prop

### SpinButton

- Simplified props to `ISpinButtonStyles` to include only the parts of the component to bring inline with
other components.
- Replaced `getClassNames` legacy prop with `styles` prop to bring component consistent to other components
and improve cachability of internal styles.


### Checkbox ### Checkbox


Expand All @@ -24,10 +16,27 @@
### Coachmark ### Coachmark


- Removed `isBeaconAnimating` and `isMeasured` style props - Removed `isBeaconAnimating` and `isMeasured` style props
- Beak:
- Removed empty `IBeak` interface
- Removed `componentRef` prop

### DatePicker

TODO: Diff of OUFR vs date-time DatePicker


### Pivot ### Pivot


- Removed deprecated and redundant props from v7, including: `initialSelectedKey` and `defaultSelectedIndex`. Use `selectedKey` or `defaultSelectedKey` to define the selected tab, and provide `itemKey` on pivot item children. - Removed deprecated and redundant props from v7, including: `initialSelectedKey` and `defaultSelectedIndex`. Use `selectedKey` or `defaultSelectedKey` to define the selected tab, and provide `itemKey` on pivot item children.
- TODO: enumerate all removed props

### Slider

TODO: document any API or functionality changes

### SpinButton

- Simplified props to `ISpinButtonStyles` to include only the parts of the component to bring inline with other components.
- Replaced `getClassNames` legacy prop with `styles` prop to bring component consistent to other components and improve cachability of internal styles.


### Others ### Others


Expand All @@ -45,7 +54,7 @@


- Updated enums to string union type: `PivotLinkFormat`, `PivotLinkSize`. (#13370) - Updated enums to string union type: `PivotLinkFormat`, `PivotLinkSize`. (#13370)


## Changes worth callout ## Other notable changes


- `styles` prop backward compat solution. - `styles` prop backward compat solution.
- css variables and IE 11 solution. - css variables and IE 11 solution.
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
import { Popup } from '../../Popup'; import { Popup } from '../../Popup';
import { classNamesFunction } from '../../Utilities'; import { classNamesFunction } from '../../Utilities';
import { AnimationClassNames } from '../../Styling'; import { AnimationClassNames } from '../../Styling';
import { useMergedRefs, useAsync } from '@uifabric/react-hooks'; import { useMergedRefs, useAsync, useConst } from '@uifabric/react-hooks';


const ANIMATIONS: { [key: number]: string | undefined } = { const ANIMATIONS: { [key: number]: string | undefined } = {
[RectangleEdge.top]: AnimationClassNames.slideUpIn10, [RectangleEdge.top]: AnimationClassNames.slideUpIn10,
Expand Down Expand Up @@ -142,7 +142,7 @@ function useBounds(
cachedBounds.current = currentBounds; cachedBounds.current = currentBounds;
} }
return cachedBounds.current; return cachedBounds.current;
}, [bounds, minPagePadding, target]); }, [bounds, minPagePadding, target, targetRef, targetWindowRef]);


return getBounds; return getBounds;
} }
Expand Down Expand Up @@ -174,6 +174,12 @@ function useMaxHeight(
} else if (hidden) { } else if (hidden) {
setMaxHeight(undefined); setMaxHeight(undefined);
} }
// TODO: check with Michael
// - Missing dependencies: coverTarget, directionalHintFixed, isBeakVisible, maxHeight
// (and immutable values async, targetRef)
// - "Mutable values like 'targetRef.current' aren't valid dependencies because mutating them
// doesn't re-render the component"
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [targetRef.current, gapSpace, beakWidth, directionalHint, getBounds, hidden]); }, [targetRef.current, gapSpace, beakWidth, directionalHint, getBounds, hidden]);


return maxHeight; return maxHeight;
Expand Down Expand Up @@ -215,6 +221,9 @@ function useHeightOffset({ finalHeight, hidden }: ICalloutProps, calloutElement:
if (!hidden) { if (!hidden) {
setHeightOffsetEveryFrame(); setHeightOffsetEveryFrame();
} }
// TODO: check with Michael
// (missing dep on setHeightOffsetEveryFrame)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [finalHeight, hidden]); }, [finalHeight, hidden]);


return heightOffset; return heightOffset;
Expand Down Expand Up @@ -275,6 +284,10 @@ function usePositions(


return () => async.cancelAnimationFrame(timerId); return () => async.cancelAnimationFrame(timerId);
} }
// TODO: check with Michael
// missing deps finalHeight, getBounds, onPositioned, positions, props, target
// (and immutable values async, calloutElement, hostElement, targetRef)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [hidden, directionalHint]); }, [hidden, directionalHint]);


return positions; return positions;
Expand All @@ -289,16 +302,21 @@ function useAutoFocus(
calloutElement: React.RefObject<HTMLDivElement>, calloutElement: React.RefObject<HTMLDivElement>,
) { ) {
const async = useAsync(); const async = useAsync();
const hasPositions = !!positions;
React.useEffect(() => { React.useEffect(() => {
if (!hidden && setInitialFocus && positions && calloutElement.current) { if (!hidden && setInitialFocus && hasPositions && calloutElement.current) {
const timerId = async.requestAnimationFrame( const timerId = async.requestAnimationFrame(
() => focusFirstChild(calloutElement.current!), () => focusFirstChild(calloutElement.current!),
calloutElement.current, calloutElement.current,
); );


return () => async.cancelAnimationFrame(timerId); return () => async.cancelAnimationFrame(timerId);
} }
}, [hidden, !!positions]); // TODO: check with Michael
// missing dep on setInitialFocus
// (and immutable values async, calloutElement)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [hidden, hasPositions]);
} }


/** /**
Expand All @@ -314,14 +332,16 @@ function useDismissHandlers(
const isMouseDownOnPopup = React.useRef(false); const isMouseDownOnPopup = React.useRef(false);
const async = useAsync(); const async = useAsync();


const mouseDownOnPopup = () => { const mouseDownHandlers = useConst([
() => {
isMouseDownOnPopup.current = true; isMouseDownOnPopup.current = true;
}; },

() => {
const mouseUpOnPopup = () => {
isMouseDownOnPopup.current = false; isMouseDownOnPopup.current = false;
}; },
] as const);


React.useEffect(() => {
const dismissOnScroll = (ev: Event) => { const dismissOnScroll = (ev: Event) => {
if (positions && !preventDismissOnScroll) { if (positions && !preventDismissOnScroll) {
dismissOnClickOrScroll(ev); dismissOnClickOrScroll(ev);
Expand Down Expand Up @@ -362,7 +382,6 @@ function useDismissHandlers(
} }
}; };


React.useEffect(() => {
// This is added so the callout will dismiss when the window is scrolled // This is added so the callout will dismiss when the window is scrolled
// but not when something inside the callout is scrolled. The delay seems // but not when something inside the callout is scrolled. The delay seems
// to be required to avoid React firing an async focus event in IE from // to be required to avoid React firing an async focus event in IE from
Expand All @@ -381,15 +400,43 @@ function useDismissHandlers(
}; };
} }
}, 0); }, 0);
// TODO: check with Michael
// missing deps on onDismiss, positions, preventDismissOnLostFocus, preventDismissOnResize, preventDismissOnScroll
// (and immutable values async, hostElement, targetRef, targetWindowRef
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [hidden]); }, [hidden]);


return [mouseDownOnPopup, mouseUpOnPopup] as const; return mouseDownHandlers;
} }


export const CalloutContentBase = React.memo( export const CalloutContentBase = React.memo(
React.forwardRef((propsWithoutDefaults: ICalloutProps, forwardedRef: React.Ref<HTMLDivElement>) => { React.forwardRef((propsWithoutDefaults: ICalloutProps, forwardedRef: React.Ref<HTMLDivElement>) => {
const props = getPropsWithDefaults(DEFAULT_PROPS, propsWithoutDefaults); const props = getPropsWithDefaults(DEFAULT_PROPS, propsWithoutDefaults);


const {
styles,
style,
ariaLabel,
ariaDescribedBy,
ariaLabelledBy,
className,
isBeakVisible,
children,
beakWidth,
calloutWidth,
calloutMaxWidth,
finalHeight,
hideOverflow = !!finalHeight,
backgroundColor,
calloutMaxHeight,
onScroll,
// eslint-disable-next-line deprecation/deprecation
shouldRestoreFocus = true,
target,
hidden,
onLayerMounted,
} = props;

const hostElement = React.useRef<HTMLDivElement>(null); const hostElement = React.useRef<HTMLDivElement>(null);
const calloutElement = React.useRef<HTMLDivElement>(null); const calloutElement = React.useRef<HTMLDivElement>(null);
const rootRef = useMergedRefs(hostElement, forwardedRef); const rootRef = useMergedRefs(hostElement, forwardedRef);
Expand All @@ -410,35 +457,16 @@ export const CalloutContentBase = React.memo(
useAutoFocus(props, positions, calloutElement); useAutoFocus(props, positions, calloutElement);


React.useEffect(() => { React.useEffect(() => {
if (!props.hidden) { if (!hidden) {
props.onLayerMounted?.(); onLayerMounted?.();
} }
}, [props.hidden]); // eslint-disable-next-line react-hooks/exhaustive-deps -- should only run if hidden changes
}, [hidden]);

// If there is no target window then we are likely in server side rendering and we should not render anything. // If there is no target window then we are likely in server side rendering and we should not render anything.
if (!targetWindowRef.current) { if (!targetWindowRef.current) {
return null; return null;
} }
const {
styles,
style,
ariaLabel,
ariaDescribedBy,
ariaLabelledBy,
className,
isBeakVisible,
children,
beakWidth,
calloutWidth,
calloutMaxWidth,
finalHeight,
hideOverflow = !!finalHeight,
backgroundColor,
calloutMaxHeight,
onScroll,
// eslint-disable-next-line deprecation/deprecation
shouldRestoreFocus = true,
target,
} = props;


const getContentMaxHeight: number | undefined = maxHeight ? maxHeight + heightOffset : undefined; const getContentMaxHeight: number | undefined = maxHeight ? maxHeight + heightOffset : undefined;
const contentMaxHeight: number | undefined = const contentMaxHeight: number | undefined =
Expand Down
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const useCheckbox = (props: ICheckboxProps, forwardedRef: React.Ref<HTMLD


function useDebugWarning(props: ICheckboxProps) { function useDebugWarning(props: ICheckboxProps) {
if (process.env.NODE_ENV !== 'production') { if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks -- build-time conditional
useWarnings({ useWarnings({
name: 'Checkbox', name: 'Checkbox',
props, props,
Expand Down Expand Up @@ -116,6 +117,6 @@ function useComponentRef(
} }
}, },
}), }),
[isChecked, isIndeterminate], [checkBoxRef, isChecked, isIndeterminate],
); );
} }
Loading