Add AbortSignal to OpenSRPServer service #625

peterMuriuki · 2019-12-16T14:34:19Z

closes #554, close #356

Refactors the openSRPService to include an abort signal that can be used to abort pending Promises.
Adds growl to a few components.
~~Add cleanup functions to Functional components that aborts fetch calls to opensrp~~ reason : 16eba72

cKellyDesign

@p-netm this is looking great, love how you're using the useEffect return to abort the signal! Just a few requests below:

src/containers/forms/OrganizationSelect/index.tsx

src/containers/pages/IRS/assignments/index.tsx

src/containers/pages/InterventionPlan/PlanDefinitionList/index.tsx

src/containers/pages/InterventionPlan/UpdatePlan/index.tsx

cKellyDesign · 2020-01-03T20:47:50Z

src/containers/pages/PractitionerViews/PractitionerListView/index.tsx

+    return () => {
+      controller.abort();
+    };


Might it be DRY'er to have a util function that does this instead of re-writing the anon function?

const controllerAborter = (controller) => { controller.abort() }; ... useEffect(() => { ... return controllerAborter(controller); }, []);

(but with a better name than controllerAborter?)

the goal for abortFetch was to prevent the error of setStating on an unmounted component. Meaning we only truly need abortFetch in components whose async operations' callbacks have a setState call. For most other components the async operations dispatch the response to the store other than call a setState in the callback. i think we should retain the callback's functionality since in this case it will not cause the aforementioned error and also I believe we might want for resolved data to be added to store if even if we unmount a component.

Refactor container tests that assumed mandatory usage of Abortsignal

coveralls · 2020-04-16T09:12:42Z

Coverage decreased (-0.006%) to 76.795% when pulling c33e1cb on 554-abortFetch into 5b0a408 on master.

moshthepitt · 2020-04-17T08:02:35Z

src/containers/pages/InterventionPlan/UpdatePlan/index.tsx

-  if (loading === true) {
+  if (plan === null) {


why this change?

moshthepitt · 2020-04-17T08:03:04Z

src/containers/pages/OrganizationViews/helpers/tests/servicehooks.test.ts

-    ).catch(e => {
-      throw e;
-    });
+    ).catch(err => err);


Do we want to use displayError here?

Naah!, this is part of the test, I want the error to actually fail the test.

moshthepitt

Why are we not using the official OpenSRP service package?

moshthepitt · 2020-04-17T08:04:55Z

src/components/forms/PractitionerForm/UserIdSelect/index.tsx

-    const allOpenMRSUsers = await loadOpenMRSUsers();
+    )
+      .list()
+      .catch((err: Error) => displayError(err));


Do we want to add a test that displayError is, in fact, called?

moshthepitt · 2020-04-17T08:05:51Z

src/containers/pages/IRS/assignments/index.tsx

@@ -99,7 +98,7 @@ const IRSAssignmentPlansList = (props: PlanAssignmentsListProps) => {
    loadData().catch(error => displayError(error));
  }, []);

-  if (loading) {
+  if (plans.length < 1) {


If we get 0 plans from the API, does this mean we will show the loading indicator forever?

i was thinking about this wrong. let me refactor.

moshthepitt · 2020-04-17T08:05:58Z

src/containers/pages/InterventionPlan/PlanDefinitionList/index.tsx

    }
  }

  useEffect(() => {
    loadData().catch(err => displayError(err));
  }, []);

-  if (loading === true) {
+  if (plans.length < 1) {


If we get 0 plans from the API, does this mean we will show the loading indicator forever?

moshthepitt · 2020-10-30T08:47:16Z

@p-netm can we add notes on how to proceed, and then possibly close this?

moshthepitt · 2021-03-22T06:25:46Z

Going to close this.

peterMuriuki requested review from cKellyDesign and FrankApiyo December 19, 2019 13:13

peterMuriuki changed the title ~~{WIP}Abort pending openSRPservice api calls on component unmount~~ Abort pending openSRPservice api calls on component unmount Dec 19, 2019

cKellyDesign suggested changes Jan 3, 2020

View reviewed changes

moshthepitt mentioned this pull request Jan 31, 2020

[PlanDefinitionList] setState on unmounted component #385

Closed

cKellyDesign assigned peterMuriuki Feb 7, 2020

peterMuriuki added 17 commits April 8, 2020 10:13

Add abort signal parameter to opensrpservice

b73a191

Add the abort signal to Practitioner view api calling funcs

a03c96b

Add cleanup to createEditPractitioner aborting pending fetch's

73fa70d

Add cleanup to PractitionerList aborting fetche's

c232cbf

Add signal param to organization views data calling funcs

37e4376

Add cleanup to Assign Practitioners aborting fetche's

99b908f

Add cleanup to create/edit org aborting fetche's

d75fd4b

Add cleanup to organization listing; aborting fetche's

15f060d

Add cleanup to single organization; aborting fetch

a2a23c1

Add cleanup to planDefinition; aborting fetch

daaede0

Add cleanup to UpdatePlan component; aborting fetch

3cb4eed

Add growling to updateplan component

319b660

Add cleanup to IRS assignments; aborting fetch

6d24bf6

Add growling and cleanup to organization select form component

64d18ea

Add cleanup and growl to UserIdSelect component

c93e1be

Refactor regression errors in organizationViews servicehooks

986710f

Fix regression errors from introducing abort

af29459

peterMuriuki force-pushed the 554-abortFetch branch from 9b467c8 to af29459 Compare April 8, 2020 09:38

peterMuriuki added 6 commits April 14, 2020 19:52

Refactor cleanup function that aborts fetch

d3a19bc

Merge branch 'master' into 554-abortFetch

c2c724b

Fix linting

abeb790

Only apply signal to fetch if its provided

652ac54

remove suprlus function abortFetch

99cd428

Refactor container tests that assumed mandatory usage of Abortsignal

peterMuriuki changed the title ~~Abort pending openSRPservice api calls on component unmount~~ Add AbortSignal to OpenSRPServer service Apr 16, 2020

peterMuriuki requested review from moshthepitt, kelvin-muchiri, ciremusyoka and kahummer and removed request for FrankApiyo April 17, 2020 07:49

moshthepitt reviewed Apr 17, 2020

View reviewed changes

moshthepitt suggested changes Apr 17, 2020

View reviewed changes

moshthepitt reviewed Apr 17, 2020

View reviewed changes

peterMuriuki added 2 commits April 17, 2020 16:36

Revert setLoading usage in Irs/assignments

237931c

Call setstaters only when component is mounted

c33e1cb

moshthepitt closed this Mar 22, 2021

moshthepitt deleted the 554-abortFetch branch March 22, 2021 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AbortSignal to OpenSRPServer service #625

Add AbortSignal to OpenSRPServer service #625

peterMuriuki commented Dec 16, 2019 •

edited

Loading

cKellyDesign left a comment

cKellyDesign Jan 3, 2020

coveralls commented Apr 16, 2020 •

edited

Loading

moshthepitt Apr 17, 2020

moshthepitt Apr 17, 2020

peterMuriuki Apr 17, 2020

moshthepitt left a comment

moshthepitt Apr 17, 2020

moshthepitt Apr 17, 2020

peterMuriuki Apr 17, 2020

moshthepitt Apr 17, 2020

moshthepitt commented Oct 30, 2020

moshthepitt commented Mar 22, 2021

Add AbortSignal to OpenSRPServer service #625

Add AbortSignal to OpenSRPServer service #625

Conversation

peterMuriuki commented Dec 16, 2019 • edited Loading

cKellyDesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshthepitt 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

moshthepitt commented Oct 30, 2020

moshthepitt commented Mar 22, 2021

peterMuriuki commented Dec 16, 2019 •

edited

Loading

coveralls commented Apr 16, 2020 •

edited

Loading