feat: data engine refactoring #115

amcgee · 2019-09-10T16:49:39Z

TRIGGER WARNING: This is not a small PR

This pull request is a major overhaul of the internal architecture of the data service. It supersedes both #66 and #102

The following are accomplished in this PR:

Add support for mutations (similar to feat: add support for mutations #66 and WIP: refactor query/mutation internals #102)
Separation of concerns between Data Engine, resource resolvers (links), and React APIs
Adds a useDataEngine hook to get direct access to the engine which supports .query and .mutation methods
Adds an engine prop to the response objects of both useDataQuery and useDataMutation, to ensure
Adds refetch callback to useDataQuery (from feat: add support for mutations #66)
94% unit test coverage (the untested bit being the re-export file)
Adds a full example application with dynamic queries (pagination), create/update/delete functionality which hits a live DHIS2 backend.
Moves query parameters into a dedicated params property, which is a breaking change but gives us much more stability and flexibility, and eliminates the potential for param naming conflicts (i.e. with the resource field)

These are the things I like about this change:

API near-parity,
Better functional isolation between components
Significantly better test coverage as a result of that isolation
Expansion potential to add and pipeline different resolver links (for caching and de-duplication, for instance, as well as automatic API version toggling)
Exposition of an imperative engine which can be used outside of React component bodies (note: the engine still needs to originate from a React context, it is not a singleton by design)

Things I don't love about this change:

Not insignificant increase in complexity - instead of everything living in one place and being pretty simple to reason about, we now have 3 layers of abstraction (which gives us a lot of benefits, but increases cognitive load for contributors and debuggers)
I worry about proliferation of data when adding the imperative interface - it will be tempting (or unconscious) to request data through the engine and then stash it somewhere (like a Redux store). This is not a good pattern, because it means duplication of memory and dilution of the "single source of truth" principle. It also makes it much harder to add caching in the future since we seed some control of the datastore.
Basically this re-invents Apollo but without GraphQL. If at some point in the future we add backend support for GraphQL, this will be wasted effort and complexity. It might be possible to implement our API access layer as a Link in Apollo, but that involves using GQL for data queries which might be a big ask.
Some bundle size bloat - this change brings bundle size from 12kb to 16kb, which is probably worth it for the features we're adding.

Layer	Consumer Context	Examples	Description
API	React >= 16.8	`DataProvider`, `useDataQuery`, `useDataEngine`, `useDataMutation`, `CustomDataProvider`	The user-facing API for applications and component libraries to access DHIS2 data. Uses React Context to initialize the Data Engine and Link, isolate application configuration, and disseminate engine access to deeply nested components.
Data Engine	Javascript	`engine.query()`, `engine.mutate()`	Processes queries and mutations and passes them to the underlying Link, returning a promise resolving to the response
Data Engine Links	Javascript	`CustomDataLink`, `RestAPILink`, `CacheLink` (future)	A link does the actual data fetching once the engine has parsed the query or mutation. Right now a single link will either generate an HTTP request or look up data in a static map, but eventually links could be chained together to do things like deduplication and cache lookups

These layers are functionally isolated in code, so there aren't any cross-layer imports (except a few Typescript types). We could split out the engine and links to separate repositories, but that will just increase the complexity at this point so I think we should defer that move until we have a reason to use the engine stand-alone.

TODO

Updated documentation
Example showing recommended use with Redux (with and without imperative calls)

varl

The technical parts of this PR looks good, it makes sense to keep the React API separate from the internals.

While it's nice to have the imperative API ready, we need to have documentation for the intended use case for it so we don't start to overuse it early and lose the benefits of declaring data up front. That would put the data engine more along the lines of D2 which was easy to leak throughout an applications architecture.

Stating the technology as "Javascript" in the matrix for the Data Engine and Data Engine Links layers should be "Typescript".

All in all I'm OK with this, I think it is time that we start applying it in the "real world" before extending it further. Especially in the refactored Maintenance app as if the Data Engine cannot deal with those use cases, then it limits its utility in the wild.

One of the most popular type of apps that is built by external developers is a maintenance app specific for their metadata and criteria.

varl · 2019-09-13T06:32:47Z

services/data/src/components/DataMutation.tsx

+    children,
+}: MutationInput) => {
+    const mutationState = useDataMutation(mutation, {
+        onCompleted,


Why choose onCompleted over onComplete?

🤷‍♂ I'd usually prefer on<verb>, but this came from Apollo Client which uses onCompleted for this exact case. Our react hooks API is almost identical to Apollo's at the moment, which I think is a nice feature. Do you think it should be onComplete?

If we want full parity another consideration is whether to use client instead of engine - I prefer engine there though I think.

amcgee · 2019-09-16T08:48:09Z

Stating the technology as "Javascript" in the matrix for the Data Engine and Data Engine Links layers should be "Typescript".

I disagree with this - the source is typescript, but the output is javascript and those layers can be consumed in any javascript context, rather than a react context. There is no dependency on typescripts for the consumer.

varl · 2019-09-16T08:50:10Z

Stating the technology as "Javascript" in the matrix for the Data Engine and Data Engine Links layers should be "Typescript".

I disagree with this - the source is typescript, but the output is javascript and those layers can be consumed in any javascript context, rather than a react context. There is no dependency on typescripts for the consumer.

Fair enough, I took the matrix as internal docs, not external so when using the matrix as a map for the review it stuck out as odd to me.

amcgee · 2019-09-16T08:54:32Z

Fair enough, I took the matrix as internal docs, not external so when using the matrix as a map for the review it stuck out as odd to me.

Changed to Consumer Context in this PR

amcgee · 2019-09-16T09:00:00Z

All in all I'm OK with this, I think it is time that we start applying it in the "real world" before extending it further. Especially in the refactored Maintenance app as if the Data Engine cannot deal with those use cases, then it limits its utility in the wild.

I agree wholeheartedly with using this in "real world", but I might urge caution in rushing to battle with maintenance-app and its variants. We can (and possibly should) provide a metadata-specific API for DHIS-heavy things (a la D2 schemas, though obviously take that comparison with a grain of salt), so trying to implement maintenance tools around a low-level data access API might lead us to "extend it further" too early. Even though there have been some 3rd party metadata management tools, I would argue actually that this is a special case of app, rather than the status quo.

amcgee · 2019-09-16T10:40:58Z

@varl renamed onCompleted to onComplete

amcgee · 2019-09-16T10:41:32Z

If approved I'll add docs and merge to next

varl · 2019-09-16T10:45:45Z

I agree wholeheartedly with using this in "real world", but I might urge caution in rushing to battle with maintenance-app and its variants.

The Maintenance App Refactor is a good candidate for that reason -- there's no real timeline to getting it "done" and retire the old maintenance app, so it is not strictly the "real world", but close enough to work as proving grounds.

amcgee · 2019-09-16T11:12:05Z

Merging this into next now, we can keep track of the remaining tasks for the v2 release in #125. Thanks @varl again for the review!

Mohammer5 · 2019-09-16T11:33:35Z

I would've preferred if this merge happened after the meeting (as I've had some questions).
Could we revert this? (Worst case we just need to merge it again)

varl · 2019-09-16T11:39:44Z

@Mohammer5 it's merged to the next branch, there is still time for questions and changes before it is merged to master.

amcgee · 2019-09-16T12:01:39Z

@Mohammer5 questions and changes still definitely welcome! I think it’s probably easier to iterate on the next branch than to discuss smaller parts of this large Pr, but if you disagree or think the overall refactor has fundamental issues I can also revert this or just not merge the next branch to master. Envoyé de mon iPhone

…

Le 16 sept. 2019 à 13:39, Viktor Varland ***@***.***> a écrit : @Mohammer5 it's merged to the next branch, there is still time for questions and changes before it is merged to master. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

Mohammer5 · 2019-09-16T12:23:38Z

@amcgee Thanks, I'll wait for the meeting before I comment on this one

* feat: data engine refactoring (#115) * feat: support network request aborting * fix: also include alert() in DataQuery renderprop * fix: don't expose imperative abort, refactor useDataQuery * feat: add ability to refetch data, upgrade react, tests * chore: run yarn autoclean for all type deps * feat: implement mutation hook * feat: add Mutation component and remove support for concurrent mutations * chore: mutation tests and bugfix * chore: fix bad merge * feat: add support for variables, callbacks, multiple mutations, and more * chore: refactor internals * fix: update fetchData call to use object parameter * fix: remove unnecessary immediate check * chore: update mutation static input label * feat: initial overhaul commit * chore: fix bugs, all tests pass, example works * chore: full test coverage * feat: add engine output to hooks, update component wrappers * chore: fix tests * chore: update types reference * chore: fix capitalization typo * chore: build before testing * chore: export useDataEngine, move dev deps to root * chore: rename onCompleted to onComplete * chore: don't collect coverage from reexport index * chore: re-indent package.json

# [1.6.0](v1.5.1...v1.6.0) (2019-09-24) ### Features * mutations, dynamic queries, engine refactor ([#127](#127)) ([8677972](8677972)), closes [#115](#115)

amcgee added 24 commits August 8, 2019 19:50

feat: support network request aborting

dc09d93

Merge branch 'master' into feat/abort

d09cb64

fix: also include alert() in DataQuery renderprop

b01ae74

Merge remote-tracking branch 'origin/master' into feat/abort

2a4ae6b

fix: don't expose imperative abort, refactor useDataQuery

9c05fd0

feat: add ability to refetch data, upgrade react, tests

940802f

chore: run yarn autoclean for all type deps

63098f9

feat: implement mutation hook

d55679f

feat: add Mutation component and remove support for concurrent mutations

3c1b5cd

chore: mutation tests and bugfix

e35db0a

Merge branch 'master' into feat/mutations

2d505f6

chore: fix bad merge

3f5500d

Merge branch 'master' into feat/mutations

eb82c1a

Merge branch 'master' into feat/mutations

1a56557

feat: add support for variables, callbacks, multiple mutations, and more

729edff

chore: refactor internals

c35cb55

fix: update fetchData call to use object parameter

15adc3a

fix: remove unnecessary immediate check

1484bbb

chore: update mutation static input label

1216ec6

feat: initial overhaul commit

5c49ec0

chore: fix bugs, all tests pass, example works

e58ae51

chore: full test coverage

a91172d

Merge branch 'next' into feat/overhaul

819ae77

feat: add engine output to hooks, update component wrappers

a868f95

amcgee requested a review from varl September 10, 2019 16:49

amcgee added 5 commits September 10, 2019 19:05

chore: fix tests

551c76c

chore: update types reference

d93c8d2

chore: fix capitalization typo

3d3f126

chore: build before testing

5a0705b

chore: export useDataEngine, move dev deps to root

d5faa97

This was referenced Sep 11, 2019

Data Engine use cases & API design dhis2/notes#56

Closed

WIP: refactor query/mutation internals #102

Closed

feat: add support for mutations #66

Closed

amcgee changed the title ~~feat: internals overhaul~~ feat: data engine refactoring Sep 11, 2019

varl reviewed Sep 16, 2019

View reviewed changes

chore: rename onCompleted to onComplete

344bc99

varl approved these changes Sep 16, 2019

View reviewed changes

amcgee mentioned this pull request Sep 16, 2019

The Road to V2 (and beyond) #125

Closed

14 tasks

amcgee merged commit 0186428 into next Sep 16, 2019

amcgee deleted the feat/overhaul branch September 16, 2019 11:12

amcgee mentioned this pull request Sep 23, 2019

feat: mutations, dynamic queries, engine refactor #127

Merged

7 tasks

dhis2-bot added a commit that referenced this pull request Sep 24, 2019

chore(release): cut 1.6.0 [skip ci]

c270e0d

# [1.6.0](v1.5.1...v1.6.0) (2019-09-24) ### Features * mutations, dynamic queries, engine refactor ([#127](#127)) ([8677972](8677972)), closes [#115](#115)

amcgee mentioned this pull request Nov 18, 2019

TypeScript for static typing dhis2/notes#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: data engine refactoring #115

feat: data engine refactoring #115

amcgee commented Sep 10, 2019 •

edited

Loading

varl left a comment

varl Sep 13, 2019

amcgee Sep 16, 2019 •

edited

Loading

amcgee commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019

Mohammer5 commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019 via email

Mohammer5 commented Sep 16, 2019

feat: data engine refactoring #115

feat: data engine refactoring #115

Conversation

amcgee commented Sep 10, 2019 • edited Loading

varl left a comment

Choose a reason for hiding this comment

varl Sep 13, 2019

Choose a reason for hiding this comment

amcgee Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

amcgee commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

amcgee commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019

Mohammer5 commented Sep 16, 2019

varl commented Sep 16, 2019

amcgee commented Sep 16, 2019 via email

Mohammer5 commented Sep 16, 2019

amcgee commented Sep 10, 2019 •

edited

Loading

amcgee Sep 16, 2019 •

edited

Loading