feat: support network request aborting and refetching

[amcgee]

This automatically aborts requests when the component is unmounted, the query changed, or a refetch initiated (new feature 👍).

This also adds eslint-plugin-react-hooks to eslint, allowing us to better check hooks. **We should probably add these to cli-style!

This does not expose an imperative abort interface, though that could be added later if there's a need for it. As it stands, I think it's better to let the component lifecycle handle the inflight requests, especially since we might not always have 1-to-1 query-to-request relationships.

We were missing dependencies for the useEffect in useDataQuery, which I've added here, so now the data will be re-requested if the query changes.

[amcgee]

@varl is there a reason we aren't using any eslint plugins (most notable eslint-plugin-react) in cli-style? Are we trying to keep it uber-generic?

[varl]

@varl is there a reason we aren't using any eslint plugins (most notable eslint-plugin-react) in cli-style? Are we trying to keep it uber-generic?

Keeping it "JavaScript" generic:

The intended scope for d2-style is that JavaScript code standards should apply to all JavaScript code, not just React.

We could add a group for React though, so you get a different ESLint configuration by doing e.g. d2-style setup project/react.

The "base" ESLint should be kept agnostic and only enforce vanilla JavaScript standards.

[amcgee]

Keeping it "JavaScript" generic

Cool cool, though so. I think having a React group might make sense, but for now I've just added some eslint React rules here.

@varl thoughts this change and particularly whether or not to include an imperative abort() option in the hook output?

[varl]

Keeping it "JavaScript" generic

Cool cool, though so. I think having a React group might make sense, but for now I've just added some eslint React rules here.

Agreed. The reason we don't have it yet is this:

One thing to think about in that case is how to install the required dependencies.

Do we want those in cli-style (can lead to bloat if we support a lot of custom projects)? Maybe the tool will tell you want deps you need to satisfy in your project instead.

[varl]

@varl thoughts this change and particularly whether or not to include an imperative abort() option in the hook output?

Generally it would be nice to not have to think about it, but there might be scenarios where we do need to manually abort a request. Unless we actually need it now, my vote goes to only aborting when the component unmounts for now and adding it when we have use case we want to support.

[amcgee]

@varl thoughts this change and particularly whether or not to include an imperative abort() option in the hook output?

Generally it would be nice to not have to think about it, but there might be scenarios where we do need to manually abort a request. Unless we actually need it now, my vote goes to only aborting when the component unmounts for now and adding it when we have use case we want to support.

Cool, I think I agree. I'll leave it out for now, will be adding imperative refetch shortly which is probably more useful and has actually been requested

[amcgee]

@varl updated to remove the imperative abort for now. Also refactored the hook implementation to be cleaner and prepare for refetch

[varl]

Looks good except for the typo. Fix that and .

[amcgee]

There's a known issue with duplicate typings in react/react-dom with yarn, fixed by adding @types/**/node_modules to .yarnclean

Added refetch feature

[amcgee]

@varl thanks for the reviews! Fixed the typo.

I went ahead and added the refetch feature here so that we don't need a second minor version bump, so I dismissed your approval.

I added an imperative refetch interface, cleaned up several sections, added tests, and upgraded the devDep on React/React-Dom to 16.9 so we don't have warnings flooding test output when testing async logic with act. I've left the peerDep in the runtime package at ^16.8.6 though, which 16.9 satisfies.

[varl]

LGTM

[amcgee]

@HendrikThePendric made a good point about AbortController in IE11 - we'll either need to polyfill it (not difficult) or guarantee that this will only be used in 2.33+

[amcgee]

(Note: IE 11 also doesn't have fetch, so we're presumably polyfilling that already in cases when this is used for pre-2.33 apps/libs like Headerbar? @varl?)

[HendrikThePendric]

Left one comment reg. IE11 support

[HendrikThePendric]

IE11 lacks support for the AbortController. From DHIS 2 version 2.33 we will drop support for IE 11, so most apps using this hook won't suffer from this problem. However, there may be some exceptions:

• Apps that transitioned to feature toggling prior to 2.33: if at a later stage a developer decides to introduce the DataQuery component, this could cause problems (they can probably be avoided somehow)
• The new HeaderBar component: I believe this uses the DataQuery too, and it is being used in a lot of v2.32 apps. So this could be another potential source of problems.

My preferred solution would be to do the following:

• Keep the code as it is
• Add a section to the README (preferably fairly prominently displayed) mentioning:
  • That this lib relies on the Fetch, AbortController and AbortSignal APIs which are unsupported in IE11
  • If your app requires IE11 support, add a polyfill for all these APIs
  • Include some instructions on how to do so, i.e. linking to whatwg-fetch and abortcontroller-polyfill

This way each app can handle things as it sees fit.

[varl]

@HendrikThePendric @amcgee We use a similar approach in ui-core where the App needs to provide polyfills: https://github.com/dhis2/ui-core#requires-polyfills

[amcgee]

Great thanks @varl @HendrikThePendric! I’ll update the readme and merge tomorrow

Would be SO COOL to formally expose lib poly fill reqs to consuming apps! One day...

[amcgee]

Merging abort and refetch for a 1.5 release!

[dhis2-bot]

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀