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

chore: Enable ESLint rules of hooks + fix new lint errors #5714

Merged
merged 3 commits into from
Oct 20, 2021
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
4 changes: 3 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module.exports = {
'eslint:recommended',
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
'plugin:react-hooks/recommended',
'airbnb',
'prettier',
'prettier/react',
Expand All @@ -41,6 +42,8 @@ module.exports = {
},
plugins: ['react-hooks', 'header'],
rules: {
'react-hooks/rules-of-hooks': ERROR,
'react-hooks/exhaustive-deps': ERROR,
'class-methods-use-this': OFF, // It's a way of allowing private variables.
'func-names': OFF,
// Ignore certain webpack alias because it can't be resolved
Expand Down Expand Up @@ -77,7 +80,6 @@ module.exports = {
'react/destructuring-assignment': OFF, // Too many lines.
'react/prefer-stateless-function': WARNING,
'react/jsx-props-no-spreading': OFF,
'react-hooks/rules-of-hooks': ERROR,
'react/require-default-props': [ERROR, {ignoreFunctionalComponents: true}],
'@typescript-eslint/no-inferrable-types': OFF,
'import/first': OFF,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function DocPageContent({
setHiddenSidebar(false);
}

setHiddenSidebarContainer(!hiddenSidebarContainer);
setHiddenSidebarContainer((value) => !value);
}, [hiddenSidebar]);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function useAutoExpandActiveCategory({
if (justBecameActive && collapsed) {
setCollapsed(false);
}
}, [isActive, wasActive, collapsed]);
}, [isActive, wasActive, collapsed, setCollapsed]);
}

function DocSidebarItemCategory({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ function DropdownNavbarItemMobile({
if (containsActive) {
setCollapsed(!containsActive);
}
}, [localPathname, containsActive]);
}, [localPathname, containsActive, setCollapsed]);

return (
<li
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ const useTheme = (): useThemeReturns => {
} catch (err) {
console.error(err);
}
}, [setTheme]);
}, [disableSwitch, setTheme]);

useEffect(() => {
if (disableSwitch && !respectPrefersColorScheme) {
Expand All @@ -80,7 +80,7 @@ const useTheme = (): useThemeReturns => {
.addListener(({matches}) => {
setTheme(matches ? themes.dark : themes.light);
});
}, []);
}, [disableSwitch, respectPrefersColorScheme]);

return {
isDarkTheme: theme === themes.dark,
Expand Down
5 changes: 5 additions & 0 deletions packages/docusaurus-theme-common/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,8 @@ export {
useScrollPosition,
useScrollPositionBlocker,
} from './utils/scrollUtils';

export {
useIsomorphicLayoutEffect,
useDynamicCallback,
} from './utils/reactUtils';
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,14 @@ const useAnnouncementBarContextValue = (): AnnouncementBarAPI => {
if (isNewAnnouncement || !isDismissedInStorage()) {
setClosed(false);
}
}, []);
}, [announcementBar]);

return useMemo(() => {
return {
isActive: !!announcementBar && !isClosed,
close: handleClose,
};
}, [isClosed]);
}, [announcementBar, isClosed, handleClose]);
};

const AnnouncementBarContext = createContext<AnnouncementBarAPI | null>(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ function useContextValue() {
return {
savePreferredVersion,
};
}, [setState]);
}, [versionPersistence]);

return [state, api] as const;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function useDocsPreferredVersion(
(versionName: string) => {
api.savePreferredVersion(pluginId, versionName);
},
[api],
[api, pluginId],
);

return {preferredVersion, savePreferredVersionName} as const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ function useShallowMemoizedObject<O extends Record<string, unknown>>(obj: O) {
return useMemo(
() => obj,
// Is this safe?
// eslint-disable-next-line react-hooks/exhaustive-deps
[...Object.keys(obj), ...Object.values(obj)],
);
}
Expand Down
34 changes: 34 additions & 0 deletions packages/docusaurus-theme-common/src/utils/reactUtils.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {useCallback, useEffect, useLayoutEffect, useRef} from 'react';

// This hook is like useLayoutEffect, but without the SSR warning
// It seems hacky but it's used in many React libs (Redux, Formik...)
// Also mentioned here: https://github.com/facebook/react/issues/16956
// It is useful when you need to update a ref as soon as possible after a React render (before useEffect)
export const useIsomorphicLayoutEffect =
typeof window !== 'undefined' ? useLayoutEffect : useEffect;

// Permits to transform an unstable callback (like an arrow function provided as props)
// to a "stable" callback that is safe to use in a useEffect dependency array
// Useful to avoid React stale closure problems + avoid useless effect re-executions
//
// Workaround until the React team recommends a good solution, see https://github.com/facebook/react/issues/16956
// This generally works has some potential drawbacks, such as https://github.com/facebook/react/issues/16956#issuecomment-536636418
export function useDynamicCallback<T extends (...args: never[]) => unknown>(
callback: T,
): T {
const ref = useRef<T>(callback);

useIsomorphicLayoutEffect(() => {
ref.current = callback;
}, [callback]);

// @ts-expect-error: TODO, not sure how to fix this TS error
return useCallback<T>((...args) => ref.current(...args), []);
}
36 changes: 22 additions & 14 deletions packages/docusaurus-theme-common/src/utils/scrollUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import React, {
useMemo,
useRef,
} from 'react';
import {useDynamicCallback} from './reactUtils';
import ExecutionEnvironment from '@docusaurus/ExecutionEnvironment';

/**
Expand Down Expand Up @@ -103,20 +104,22 @@ export function useScrollPosition(
const {scrollEventsEnabledRef} = useScrollController();
const lastPositionRef = useRef<ScrollPosition | null>(getScrollPosition());

const handleScroll = () => {
if (!scrollEventsEnabledRef.current) {
return;
}
const currentPosition = getScrollPosition()!;
const dynamicEffect = useDynamicCallback(effect);

if (effect) {
effect(currentPosition, lastPositionRef.current);
}
useEffect(() => {
const handleScroll = () => {
if (!scrollEventsEnabledRef.current) {
return;
}
const currentPosition = getScrollPosition()!;

lastPositionRef.current = currentPosition;
};
if (dynamicEffect) {
dynamicEffect(currentPosition, lastPositionRef.current);
}

lastPositionRef.current = currentPosition;
};

useEffect(() => {
const opts: AddEventListenerOptions & EventListenerOptions = {
passive: true,
};
Expand All @@ -125,7 +128,12 @@ export function useScrollPosition(
window.addEventListener('scroll', handleScroll, opts);

return () => window.removeEventListener('scroll', handleScroll, opts);
}, deps);
}, [
dynamicEffect,
scrollEventsEnabledRef,
// eslint-disable-next-line react-hooks/exhaustive-deps
...deps,
]);
}

type UseScrollPositionSaver = {
Expand Down Expand Up @@ -170,7 +178,7 @@ function useScrollPositionSaver(): UseScrollPositionSaver {
return {restored: heightDiff !== 0};
}, []);

return useMemo(() => ({save, restore}), []);
return useMemo(() => ({save, restore}), [restore, save]);
}

type UseScrollPositionBlockerReturn = {
Expand Down Expand Up @@ -217,7 +225,7 @@ export function useScrollPositionBlocker(): UseScrollPositionBlockerReturn {
}
};
},
[scrollController],
[scrollController, scrollPositionSaver],
);

useLayoutEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {useEffect} from 'react';
import {useLocation} from '@docusaurus/router';
import {Location} from '@docusaurus/history';
import {usePrevious} from './usePrevious';
import {useDynamicCallback} from './reactUtils';

type LocationChangeEvent = {
location: Location;
Expand All @@ -21,10 +22,12 @@ export function useLocationChange(onLocationChange: OnLocationChange): void {
const location = useLocation();
const previousLocation = usePrevious(location);

const onLocationChangeDynamic = useDynamicCallback(onLocationChange);

useEffect(() => {
onLocationChange({
onLocationChangeDynamic({
Copy link
Contributor

Choose a reason for hiding this comment

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

@slorber I just noticed that this change negatively affects on the hideable sidebar feature. After location change (I suppose), while closing a sidebar, the current scroll position is reset to zero, i.e. a page scrolls to the top.

Actual behavior:

ezgif.com-gif-maker.mp4

Expected behavior

ezgif.com-gif-maker.1.mp4

Could you please see how this can be fixed?

location,
previousLocation,
});
}, [location]);
}, [onLocationChangeDynamic, location, previousLocation]);
}
5 changes: 3 additions & 2 deletions packages/docusaurus-theme-common/src/utils/usePrevious.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@
* LICENSE file in the root directory of this source tree.
*/

import {useRef, useEffect} from 'react';
import {useRef} from 'react';
import {useIsomorphicLayoutEffect} from './reactUtils';

export function usePrevious<T>(value: T): T | undefined {
const ref = useRef<T>();

useEffect(() => {
useIsomorphicLayoutEffect(() => {
ref.current = value;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import clsx from 'clsx';
import Head from '@docusaurus/Head';
import Link from '@docusaurus/Link';
import ExecutionEnvironment from '@docusaurus/ExecutionEnvironment';
import {useTitleFormatter, usePluralForm} from '@docusaurus/theme-common';
import {
useTitleFormatter,
usePluralForm,
useDynamicCallback,
} from '@docusaurus/theme-common';
import useDocusaurusContext from '@docusaurus/useDocusaurusContext';
import {useAllDocsData} from '@theme/hooks/useDocs';
import useSearchQuery from '@theme/hooks/useSearchQuery';
Expand Down Expand Up @@ -173,6 +177,7 @@ function SearchPage() {
},
initialSearchResultState,
);

const algoliaClient = algoliaSearch(appId, apiKey);
const algoliaHelper = algoliaSearchHelper(algoliaClient, indexName, {
hitsPerPage: 15,
Expand Down Expand Up @@ -271,7 +276,7 @@ function SearchPage() {
description: 'The search page title for empty query',
});

const makeSearch = (page = 0) => {
const makeSearch = useDynamicCallback((page = 0) => {
algoliaHelper.addDisjunctiveFacetRefinement('docusaurus_tag', 'default');
algoliaHelper.addDisjunctiveFacetRefinement('language', currentLocale);

Expand All @@ -285,18 +290,16 @@ function SearchPage() {
);

algoliaHelper.setQuery(searchQuery).setPage(page).search();
};
});

useEffect(() => {
if (!loaderRef) {
return undefined;
}
const currentObserver = observer.current;

observer.current.observe(loaderRef);

return () => {
observer.current.unobserve(loaderRef);
};
currentObserver.observe(loaderRef);
return () => currentObserver.unobserve(loaderRef);
}, [loaderRef]);

useEffect(() => {
Expand All @@ -311,21 +314,26 @@ function SearchPage() {
makeSearch();
}, 300);
}
}, [searchQuery, docsSearchVersionsHelpers.searchVersions]);
}, [
searchQuery,
docsSearchVersionsHelpers.searchVersions,
updateSearchPath,
makeSearch,
]);

useEffect(() => {
if (!searchResultState.lastPage || searchResultState.lastPage === 0) {
return;
}

makeSearch(searchResultState.lastPage);
}, [searchResultState.lastPage]);
}, [makeSearch, searchResultState.lastPage]);

useEffect(() => {
if (searchValue && searchValue !== searchQuery) {
setSearchQuery(searchValue);
}
}, [searchValue]);
}, [searchQuery, searchValue]);

return (
<Layout wrapperClassName="search-page-wrapper">
Expand Down
16 changes: 8 additions & 8 deletions packages/docusaurus/src/client/exports/Link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -90,24 +90,24 @@ function Link({

const IOSupported = ExecutionEnvironment.canUseIntersectionObserver;

let io: IntersectionObserver;
const ioRef = useRef<IntersectionObserver>();
const handleIntersection = (el: HTMLAnchorElement, cb: () => void) => {
io = new window.IntersectionObserver((entries) => {
ioRef.current = new window.IntersectionObserver((entries) => {
entries.forEach((entry) => {
if (el === entry.target) {
// If element is in viewport, stop listening/observing and run callback.
// https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
if (entry.isIntersecting || entry.intersectionRatio > 0) {
io.unobserve(el);
io.disconnect();
ioRef.current!.unobserve(el);
ioRef.current!.disconnect();
cb();
}
}
});
});

// Add element to the observer.
io.observe(el);
ioRef.current!.observe(el);
};

const handleRef = (ref: HTMLAnchorElement | null) => {
Expand Down Expand Up @@ -138,11 +138,11 @@ function Link({

// When unmounting, stop intersection observer from watching.
return () => {
if (IOSupported && io) {
io.disconnect();
if (IOSupported && ioRef.current) {
ioRef.current.disconnect();
}
};
}, [targetLink, IOSupported, isInternal]);
}, [ioRef, targetLink, IOSupported, isInternal]);

const isAnchorLink = targetLink?.startsWith('#') ?? false;
const isRegularHtmlLink = !targetLink || !isInternal || isAnchorLink;
Expand Down