feat: refactor non instruction components #132

alangsto · 2024-02-01T17:41:18Z

COSMO-172

This PR removes the use of the ExamStateProvider component and withExamStore function, replacing any calls to context or state with a selector.

The examStore has also been renamed to specialExams in preparation for our eventual merger of the exams store into the learning MFE.

alangsto · 2024-02-02T15:26:37Z

The public API test suite is now failing, but the work to fix those tests + API will be part of COSMO-175

varshamenon4

This is awesome! Definitely really complicated stuff. Just a couple of comments and questions.

varshamenon4 · 2024-02-02T15:56:53Z

src/context.jsx

-
-const ExamStateContext = React.createContext({});
-export default ExamStateContext;
+// import React from 'react';


[question] for these parts, is there a reason to leave commented out rather than removing altogether?

I removed them now!

varshamenon4 · 2024-02-02T15:57:16Z

src/core/ExamStateProvider.jsx

-);
-
-export default ExamStateProvider;
+// import React, { useMemo } from 'react';


[question] same question as above: can these parts be removed?

varshamenon4 · 2024-02-02T16:02:30Z

src/core/OuterExamTimer.jsx

+  } = useSelector(state => state.specialExams);
+  const dispatch = useDispatch();
+
+  const showTimer = !!(activeAttempt && IS_STARTED_STATUS(activeAttempt.attempt_status));


TIL what !! is - cooool! Love "bang, bang you're boolean" here: https://stackoverflow.com/questions/784929/what-is-the-not-not-operator-in-javascript
Nice!

varshamenon4 · 2024-02-02T16:03:15Z

src/core/OuterExamTimer.jsx

+          submitExam={() => dispatch(submitExam())}
+          expireExamAttempt={() => dispatch(expireExam())}
+          pollExamAttempt={(url) => dispatch(pollAttempt(url))}
+          pingAttempt={(timeout, url) => dispatch(pingAttempt(timeout, url))}


varshamenon4 · 2024-02-02T16:10:25Z

src/exam/Exam.jsx

@@ -70,6 +77,7 @@ const Exam = ({
    // this makes sure useEffect gets called only one time after the exam has been fetched
    // we can't leave this empty since initially exam is just an empty object, so
    // API calls above would not get triggered
+    // eslint-disable-next-line react-hooks/exhaustive-deps


[question] I noticed this in a couple of places. Could you clarify the need for this / if this will eventually be unnecessary?

This lint rule is meant to try and prevent developers from forgetting dependencies in the use effect. Because I didn't change anything within the use effect, I'm planning to leave this as is for now!

Should it be removed?

I'm wondering if it's complaining because you're using dispatch in the useEffect hook, so dispatch has to be added to the dependency array.

Oh, fair point! I will try adding that in and see what happens.

MichaelRoytman

I left a few nits, but this looks great 🎉!

MichaelRoytman · 2024-02-02T18:37:46Z

src/core/OuterExamTimer.jsx

 import { ExamTimerBlock } from '../timer';
 import ExamAPIError from '../exam/ExamAPIError';
-import ExamStateProvider from './ExamStateProvider';
+import { getLatestAttemptData } from '../data/thunks';


Nit. It seems like most components import from the top level data module (things exported from data/index.js). Should we follow that pattern here and in the other files?

True, will update!

MichaelRoytman · 2024-02-02T18:39:14Z

src/data/store.js

-    specialExams: examReducer,
-  },
-});
+// import { configureStore } from '@reduxjs/toolkit';


I think we're okay to remove this, since we're still on a feature branch, right? Or were you thinking of doing that in a follow up pull request?

Let me try removing that as part of this one

Removing the store requires quite a bit of refactoring, because most of the tests directly import the store. I'll work through that now though!

Oof! I didn't realize that. If you want to leave that to another ticket, this pull request looks good to merge to me, but I'm happy to re-review if you want to refactor the tests.

It wasn't as bad as I expected because I could just use the test store as a replacement! Thankful that it existed. Should be set for another review now :)

MichaelRoytman · 2024-02-02T18:40:48Z

src/exam/Exam.jsx

+  } = useSelector(state => state.specialExams);
+  const dispatch = useDispatch();
+
+  const showTimer = !!(activeAttempt && IS_STARTED_STATUS(activeAttempt.attempt_status));


Nit. If you wanted to reduce some of the duplication in the definition of showTimer, you could put it in a custom hook.

MichaelRoytman · 2024-02-02T18:41:53Z

src/exam/Exam.jsx

@@ -70,6 +77,7 @@ const Exam = ({
    // this makes sure useEffect gets called only one time after the exam has been fetched
    // we can't leave this empty since initially exam is just an empty object, so
    // API calls above would not get triggered
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Should it be removed?

I'm wondering if it's complaining because you're using dispatch in the useEffect hook, so dispatch has to be added to the dependency array.

MichaelRoytman · 2024-02-05T14:15:01Z

src/data/store.js

@@ -1,8 +0,0 @@
-import { configureStore } from '@reduxjs/toolkit';


… useSelector hooks API. (#131) * feat: rename examStore to specialExams in preparation for use in frontend-app-learning This commit renames the examStore to specialExams. This is in preparation for the use of the exam reducer in the frontend-app-learning React application. * refactor: replace use of context with thunks and Redux store in instructions components This commit replaces the use of the ExamStateContext with the use of thunks and the Redux store in the instructions components. The original pattern was to use the withExamStore higher-order component to provide context to the instructions components. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. * test: remove references to ExamStateProvider from tests for instructions components This commit removes references to the ExamStateProvider from tests for instructions components. Because these components have been refactored to no longe rely on the ExamStateProvider, they are not required in the component tree. refactor: replace use of withExamStore higher-order component in TimerServiceProvider (#133) This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. feat: refactor non instruction components (#132) Refactor public API functions to no longer import and use store. (#134) * test: remove asynchronicity from initializing test store The initializeTestStore test utility function used async...await keywords for initializing the test store, which isn't necessary. This commit removes the async...await keywords and refactors any tests that use that function. * test: fix mocking of getExamAttemptsData function * feat: refactor public API to use hooks instead of references to exported store This commit refactors the public API, used by the frontend-app-learning application to interact with the frontend-lib-special-exams state, to export a series of hooks. Originally, the public API imported the frontend-lib-special-exams store directly and operated on it in a series of exported functions. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by public API. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. This commit also exports the root reducer from this library. This root reducer will be imported by the frontend-app-learning application and used to configure its store. fix: patch bugs found during bash (#135)

… useSelector hooks API. (#131) (#136) * feat: rename examStore to specialExams in preparation for use in frontend-app-learning This commit renames the examStore to specialExams. This is in preparation for the use of the exam reducer in the frontend-app-learning React application. * refactor: replace use of context with thunks and Redux store in instructions components This commit replaces the use of the ExamStateContext with the use of thunks and the Redux store in the instructions components. The original pattern was to use the withExamStore higher-order component to provide context to the instructions components. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. * test: remove references to ExamStateProvider from tests for instructions components This commit removes references to the ExamStateProvider from tests for instructions components. Because these components have been refactored to no longe rely on the ExamStateProvider, they are not required in the component tree. refactor: replace use of withExamStore higher-order component in TimerServiceProvider (#133) This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. feat: refactor non instruction components (#132) Refactor public API functions to no longer import and use store. (#134) * test: remove asynchronicity from initializing test store The initializeTestStore test utility function used async...await keywords for initializing the test store, which isn't necessary. This commit removes the async...await keywords and refactors any tests that use that function. * test: fix mocking of getExamAttemptsData function * feat: refactor public API to use hooks instead of references to exported store This commit refactors the public API, used by the frontend-app-learning application to interact with the frontend-lib-special-exams state, to export a series of hooks. Originally, the public API imported the frontend-lib-special-exams store directly and operated on it in a series of exported functions. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by public API. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. This commit also exports the root reducer from this library. This root reducer will be imported by the frontend-app-learning application and used to configure its store. fix: patch bugs found during bash (#135) Co-authored-by: Michael Roytman <[email protected]>

alangsto force-pushed the alangsto/update_non_instruction_components branch 2 times, most recently from fe12cb1 to d4abd91 Compare February 1, 2024 19:05

alangsto changed the base branch from 2u/cosmonauts/refactor-store to michaelroytman/COSMO-173-refactor-store-instructions February 1, 2024 19:05

alangsto force-pushed the alangsto/update_non_instruction_components branch 3 times, most recently from 235dcac to bc11f34 Compare February 2, 2024 15:22

alangsto mentioned this pull request Feb 2, 2024

Refactor TimerServiceProvider to no longer use withExamStore higher-order-component. #133

Merged

varshamenon4 reviewed Feb 2, 2024

View reviewed changes

Base automatically changed from michaelroytman/COSMO-173-refactor-store-instructions to 2u/cosmonauts/refactor-store February 2, 2024 16:29

alangsto force-pushed the alangsto/update_non_instruction_components branch 3 times, most recently from 93a1ea6 to 360bbef Compare February 2, 2024 18:22

MichaelRoytman approved these changes Feb 2, 2024

View reviewed changes

alangsto force-pushed the alangsto/update_non_instruction_components branch 2 times, most recently from 03b797a to d53cebe Compare February 2, 2024 19:30

feat: refactor non instruction components

517b94d

alangsto force-pushed the alangsto/update_non_instruction_components branch from c9e9715 to 517b94d Compare February 2, 2024 19:33

MichaelRoytman reviewed Feb 5, 2024

View reviewed changes

src/data/store.js

@@ -1,8 +0,0 @@

import { configureStore } from '@reduxjs/toolkit';

Copy link

Contributor

MichaelRoytman Feb 5, 2024

Choose a reason for hiding this comment

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

🎉

MichaelRoytman approved these changes Feb 5, 2024

View reviewed changes

alangsto merged commit 8e51de4 into 2u/cosmonauts/refactor-store Feb 5, 2024
3 of 5 checks passed

alangsto deleted the alangsto/update_non_instruction_components branch February 5, 2024 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor non instruction components #132

feat: refactor non instruction components #132

alangsto commented Feb 1, 2024

alangsto commented Feb 2, 2024

varshamenon4 left a comment

varshamenon4 Feb 2, 2024

alangsto Feb 2, 2024

varshamenon4 Feb 2, 2024

varshamenon4 Feb 2, 2024

varshamenon4 Feb 2, 2024

varshamenon4 Feb 2, 2024

alangsto Feb 2, 2024

MichaelRoytman Feb 2, 2024

alangsto Feb 2, 2024

MichaelRoytman left a comment

MichaelRoytman Feb 2, 2024

alangsto Feb 2, 2024

MichaelRoytman Feb 2, 2024

alangsto Feb 2, 2024

alangsto Feb 2, 2024

MichaelRoytman Feb 2, 2024

alangsto Feb 2, 2024

MichaelRoytman Feb 2, 2024

MichaelRoytman Feb 2, 2024

MichaelRoytman Feb 5, 2024

		@@ -1,8 +0,0 @@
		import { configureStore } from '@reduxjs/toolkit';

feat: refactor non instruction components #132

feat: refactor non instruction components #132

Conversation

alangsto commented Feb 1, 2024

alangsto commented Feb 2, 2024

varshamenon4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRoytman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment