fix enable rules of hooks (#329)

This is a PR enabling eslint "rules-of-hooks".

This lint rule catches some very annoying bugs and enforces you to use correct naming for custom hooks (prepend with "use"), which is a mandatory react rule and important for a number of reasons.

I added eslint-disable statements wherever the rules are broken, and if this is merged, I would expect new code not to break the rules any longer.

I strongly suggest that the much better way to extract and reuse hooks and logic from components is the way the community does it and the React docs suggest. The new React docs are really fantastic in this regard and you can use the patterns found there very well to have an excellent application. https://react.dev/learn/reusing-logic-with-custom-hooks
This commit is contained in:
Jesper Hodge
2023-07-05 15:11:07 -04:00
committed by GitHub
parent c09faa3b09
commit f942ef9594
32 changed files with 96 additions and 1 deletions

View File

@@ -7,7 +7,7 @@ const config = createConfig('eslint', {
'import/no-named-as-default-member': 'off',
'import/no-self-import': 'off',
'spaced-comment': ['error', 'always', { block: { exceptions: ['*'] } }],
'react-hooks/rules-of-hooks': 'off',
'react-hooks/rules-of-hooks': 2,
'react-hooks/exhaustive-deps': 'off',
'no-promise-executor-return': 'off',
radix: 'off',

View File

@@ -8,11 +8,13 @@ import * as module from './hooks';
export const { navigateCallback } = textEditorHooks;
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
localTitle: (args) => React.useState(args),
};
export const hooks = {
isEditing: () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [isEditing, setIsEditing] = React.useState(false);
return {
isEditing,
@@ -22,6 +24,7 @@ export const hooks = {
},
localTitle: ({ dispatch, stopEditing }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const title = useSelector(selectors.app.displayTitle);
const [localTitle, setLocalTitle] = module.state.localTitle(title);
return {

View File

@@ -17,7 +17,9 @@ export const TitleHeader = ({
intl,
}) => {
if (!isInitialized) { return intl.formatMessage(messages.loading); }
// eslint-disable-next-line react-hooks/rules-of-hooks
const dispatch = useDispatch();
// eslint-disable-next-line react-hooks/rules-of-hooks
const title = useSelector(selectors.app.displayTitle);
const {

View File

@@ -16,6 +16,7 @@ export const {
} = appHooks;
export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
isCancelConfirmModalOpen: (val) => useState(val),
});
@@ -25,7 +26,9 @@ export const handleSaveClicked = ({
validateEntry,
returnFunction,
}) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const destination = useSelector(selectors.app.returnUrl);
// eslint-disable-next-line react-hooks/rules-of-hooks
const analytics = useSelector(selectors.app.analytics);
return () => saveBlock({
@@ -53,14 +56,18 @@ export const handleCancel = ({ onClose, returnFunction }) => {
}
return navigateCallback({
returnFunction,
// eslint-disable-next-line react-hooks/rules-of-hooks
destination: useSelector(selectors.app.returnUrl),
analyticsEvent: analyticsEvt.editorCancelClick,
// eslint-disable-next-line react-hooks/rules-of-hooks
analytics: useSelector(selectors.app.analytics),
});
};
// eslint-disable-next-line react-hooks/rules-of-hooks
export const isInitialized = () => useSelector(selectors.app.isInitialized);
// eslint-disable-next-line react-hooks/rules-of-hooks
export const saveFailed = () => useSelector((rootState) => (
selectors.requests.isFailed(rootState, { requestKey: RequestKeys.saveBlock })
));

View File

@@ -6,6 +6,7 @@ import { ProblemTypeKeys } from '../../../../../data/constants/problem';
import { fetchEditorContent } from '../hooks';
export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
isFeedbackVisible: (val) => useState(val),
});

View File

@@ -12,10 +12,15 @@ import {
import { fetchEditorContent } from '../hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
showAdvanced: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
cardCollapsed: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
summary: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showAttempts: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
attemptDisplayValue: (val) => useState(val),
};
@@ -44,6 +49,7 @@ export const showFullCard = (hasExpandableTextArea) => {
export const hintsCardHooks = (hints, updateSettings) => {
const [summary, setSummary] = module.state.summary({ message: messages.noHintSummary, values: {} });
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const hintsNumber = hints.length;
if (hintsNumber === 0) {

View File

@@ -4,6 +4,7 @@ import messages from './messages';
import * as module from './hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
summary: (val) => useState(val),
};
@@ -12,6 +13,7 @@ export const generalFeedbackHooks = (generalFeedback, updateSettings) => {
message: messages.noGeneralFeedbackSummary, values: {}, intl: true,
});
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
if (_.isEmpty(generalFeedback)) {
setSummary({ message: messages.noGeneralFeedbackSummary, values: {}, intl: true });

View File

@@ -4,12 +4,14 @@ import messages from './messages';
import * as module from './hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
summary: (val) => useState(val),
};
export const groupFeedbackCardHooks = (groupFeedbacks, updateSettings, answerslist) => {
const [summary, setSummary] = module.state.summary({ message: messages.noGroupFeedbackSummary, values: {} });
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
if (groupFeedbacks.length === 0) {
setSummary({ message: messages.noGroupFeedbackSummary, values: {} });

View File

@@ -3,6 +3,7 @@ import { RandomizationTypes, RandomizationTypesKeys } from '../../../../../../..
import * as module from './hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
summary: (val) => useState(val),
};

View File

@@ -7,6 +7,7 @@ import { setAssetToStaticUrl } from '../../../../sharedComponents/TinyMceWidget/
import { ProblemTypeKeys } from '../../../../data/constants/problem';
export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
isSaveWarningModalOpen: (val) => useState(val),
});

View File

@@ -7,6 +7,7 @@ import * as module from './hooks';
import { getDataFromOlx } from '../../../../data/redux/thunkActions/problem';
export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
selected: (val) => useState(val),
});

View File

@@ -10,7 +10,9 @@ import * as module from './SelectVideoModal';
export const hooks = {
videoList: ({ fetchVideos }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [videos, setVideos] = React.useState(null);
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
fetchVideos({ onSuccess: setVideos });
}, []);

View File

@@ -13,12 +13,15 @@ export const {
export const hooks = {
initialize: (dispatch, selectedVideoId, selectedVideoUrl) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
dispatch(thunkActions.video.loadVideoData(selectedVideoId, selectedVideoUrl));
}, []);
},
returnToGallery: () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const learningContextId = useSelector(selectors.app.learningContextId);
// eslint-disable-next-line react-hooks/rules-of-hooks
const blockId = useSelector(selectors.app.blockId);
return () => (navigateTo(`/course/${learningContextId}/editor/course-videos/${blockId}`));
},

View File

@@ -9,8 +9,10 @@ const durationMatcher = /^(\d{0,2}):?(\d{0,2})?:?(\d{0,2})?$/i;
export const durationWidget = ({ duration, updateField }) => {
const setDuration = (val) => updateField({ duration: val });
const initialState = module.durationString(duration);
// eslint-disable-next-line react-hooks/rules-of-hooks
const [unsavedDuration, setUnsavedDuration] = useState(initialState);
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
setUnsavedDuration(module.durationString(duration));
}, [duration]);

View File

@@ -4,6 +4,7 @@ import { thunkActions } from '../../../../../../data/redux';
import * as module from './hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
showSizeError: (args) => React.useState(args),
};
@@ -28,7 +29,9 @@ export const checkValidFileSize = ({
};
export const fileInput = ({ fileSizeError }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const dispatch = useDispatch();
// eslint-disable-next-line react-hooks/rules-of-hooks
const ref = React.useRef();
const click = () => ref.current.click();
const addFile = (e) => {

View File

@@ -5,6 +5,7 @@ import * as constants from './constants';
import * as module from './hooks';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
showSizeError: (args) => React.useState(args),
};
@@ -85,7 +86,9 @@ export const checkValidSize = ({ file, onSizeFail }) => {
};
export const fileInput = ({ setThumbnailSrc, imgRef, fileSizeError }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const dispatch = useDispatch();
// eslint-disable-next-line react-hooks/rules-of-hooks
const ref = React.useRef();
const click = () => ref.current.click();
const addFile = (e) => {

View File

@@ -25,6 +25,7 @@ import messages from './messages';
export const hooks = {
state: {
// eslint-disable-next-line react-hooks/rules-of-hooks
inDeleteConfirmation: (args) => React.useState(args),
},
setUpDeleteConfirmation: () => {

View File

@@ -33,6 +33,7 @@ import * as module from './index';
export const hooks = {
updateErrors: ({ isUploadError, isDeleteError }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [error, setError] = React.useContext(ErrorContext).transcripts;
if (isUploadError) {
setError({ ...error, uploadError: messages.uploadTranscriptError.defaultMessage });

View File

@@ -4,6 +4,7 @@ import { parseYoutubeId } from '../../../../../../data/services/cms/api';
import * as requests from '../../../../../../data/redux/thunkActions/requests';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
showVideoIdChangeAlert: (args) => React.useState(args),
};

View File

@@ -74,6 +74,7 @@ export const updatedObject = (obj, index, val) => ({ ...obj, [index]: val });
* @param {string} key - form key
* @return {func} - callback taking a value and updating the video redux field
*/
// eslint-disable-next-line react-hooks/rules-of-hooks
export const updateFormField = ({ dispatch, key }) => useCallback(
(val) => dispatch(actions.video.updateField({ [key]: val })),
[],
@@ -93,14 +94,17 @@ export const updateFormField = ({ dispatch, key }) => useCallback(
* setAll - sets form field in hook AND redux
*/
export const valueHooks = ({ dispatch, key }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const formValue = useSelector(selectors.video[key]);
const [local, setLocal] = module.state[key](formValue);
const setFormValue = module.updateFormField({ dispatch, key });
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
setLocal(formValue);
}, [formValue]);
// eslint-disable-next-line react-hooks/rules-of-hooks
const setAll = useCallback(
(val) => {
setLocal(val);

View File

@@ -7,12 +7,14 @@ import * as module from './hooks';
export const ErrorContext = createContext();
export const state = StrictDict({
/* eslint-disable react-hooks/rules-of-hooks */
durationErrors: (val) => useState(val),
handoutErrors: (val) => useState(val),
licenseErrors: (val) => useState(val),
thumbnailErrors: (val) => useState(val),
transcriptsErrors: (val) => useState(val),
videoSourceErrors: (val) => useState(val),
/* eslint-enable react-hooks/rules-of-hooks */
});
export const errorsHook = () => {

View File

@@ -19,12 +19,19 @@ export const {
} = appHooks;
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
highlighted: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
searchString: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showSelectVideoError: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showSizeError: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
sortBy: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
filertBy: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
hideSelectedVideos: (val) => React.useState(val),
};
@@ -100,7 +107,9 @@ export const videoListProps = ({ searchSortProps, videos }) => {
setShowSizeError,
] = module.state.showSizeError(false);
const filteredList = module.filterList({ ...searchSortProps, videos });
// eslint-disable-next-line react-hooks/rules-of-hooks
const learningContextId = useSelector(selectors.app.learningContextId);
// eslint-disable-next-line react-hooks/rules-of-hooks
const blockId = useSelector(selectors.app.blockId);
return {
galleryError: {
@@ -146,14 +155,18 @@ export const fileInputProps = () => {
};
export const handleVideoUpload = () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const learningContextId = useSelector(selectors.app.learningContextId);
// eslint-disable-next-line react-hooks/rules-of-hooks
const blockId = useSelector(selectors.app.blockId);
return () => navigateTo(`/course/${learningContextId}/editor/video_upload/${blockId}`);
};
export const handleCancel = () => (
navigateCallback({
// eslint-disable-next-line react-hooks/rules-of-hooks
destination: useSelector(selectors.app.returnUrl),
// eslint-disable-next-line react-hooks/rules-of-hooks
analytics: useSelector(selectors.app.analytics),
analyticsEvent: analyticsEvt.videoGalleryCancelClick,
})

View File

@@ -14,8 +14,11 @@ export const {
} = appHooks;
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
loading: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
errorMessage: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
textInputValue: (val) => React.useState(val),
};

View File

@@ -7,6 +7,7 @@ import { actions, thunkActions } from './data/redux';
import * as module from './hooks';
import { RequestKeys } from './data/constants/requests';
// eslint-disable-next-line react-hooks/rules-of-hooks
export const initializeApp = ({ dispatch, data }) => useEffect(
() => dispatch(thunkActions.app.initialize(data)),
[data],

View File

@@ -13,6 +13,7 @@ import './index.scss';
const CODEMIRROR_LANGUAGES = { HTML: 'html', XML: 'xml' };
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
showBtnEscapeHTML: (val) => React.useState(val),
};
@@ -60,6 +61,7 @@ export const createCodeMirrorDomNode = ({
upstreamRef,
lang,
}) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const languageExtension = lang === CODEMIRROR_LANGUAGES.HTML ? html() : xml();
const cleanText = cleanHTML({ initialText });

View File

@@ -9,10 +9,12 @@ import messages from './messages';
export const hooks = {
state: {
// eslint-disable-next-line react-hooks/rules-of-hooks
isDismissed: (val) => React.useState(val),
},
dismissalHooks: ({ dismissError, isError }) => {
const [isDismissed, setIsDismissed] = hooks.state.isDismissed(false);
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(
() => {
setIsDismissed(isDismissed && !isError);

View File

@@ -2,6 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
export const fileInput = ({ onAddFile }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const ref = React.useRef();
const click = () => ref.current.click();
const addFile = (e) => {

View File

@@ -5,14 +5,23 @@ import * as module from './hooks';
// Simple wrappers for useState to allow easy mocking for tests.
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
altText: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
dimensions: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showAltTextDismissibleError: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showAltTextSubmissionError: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
isDecorative: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
isLocked: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
local: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
lockDims: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
lockInitialized: (val) => React.useState(val),
};

View File

@@ -30,6 +30,7 @@ const testVal = 'MY test VALUE';
describe('state values', () => {
const testStateMethod = (key) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
expect(hooks.state[key](testVal)).toEqual(React.useState(testVal));
};
test('provides altText state value', () => testStateMethod(state.keys.altText));

View File

@@ -7,10 +7,15 @@ import { sortFunctions, sortKeys, sortMessages } from './utils';
import messages from './messages';
export const state = {
// eslint-disable-next-line react-hooks/rules-of-hooks
highlighted: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showSelectImageError: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
searchString: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
sortBy: (val) => React.useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
showSizeError: (val) => React.useState(val),
};
@@ -98,7 +103,9 @@ export const checkValidFileSize = ({
};
export const fileInputHooks = ({ setSelection, clearSelection, imgList }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const dispatch = useDispatch();
// eslint-disable-next-line react-hooks/rules-of-hooks
const ref = React.useRef();
const click = () => ref.current.click();
const addFile = (e) => {

View File

@@ -12,6 +12,7 @@ export const getSaveBtnProps = ({ editorRef, ref, close }) => ({
});
export const prepareSourceCodeModal = ({ editorRef, close }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const ref = useRef();
const saveBtnProps = module.getSaveBtnProps({ editorRef, ref, close });

View File

@@ -11,9 +11,13 @@ import * as module from './hooks';
import tinyMCE from '../../data/constants/tinyMCE';
export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
isImageModalOpen: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
isSourceCodeModalOpen: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
imageSelection: (val) => useState(val),
// eslint-disable-next-line react-hooks/rules-of-hooks
refReady: (val) => useState(val),
});
@@ -232,11 +236,14 @@ export const editorConfig = ({
};
export const prepareEditorRef = () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const editorRef = useRef(null);
// eslint-disable-next-line react-hooks/rules-of-hooks
const setEditorRef = useCallback((ref) => {
editorRef.current = ref;
}, []);
const [refReady, setRefReady] = module.state.refReady(false);
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => setRefReady(true), []);
return { editorRef, refReady, setEditorRef };
};