feat: support action queries with prefixed resource names

[amcgee]

This adds first-class support for action resource types. These are legacy resources which are deprecated and should be avoided when an alternative exists, so this will not be a documented feature.

To fetch from an action endpoint such as dhis-web-commons/menu/getModules.action use the following query (note the action:: resource prefix):

const query = {
    apps: {
        resource: 'action::menu/getModules'
    }
}
const { data, loading, error } = useQuery(query)

I don't love this string-parsing API, but it's less disruptive than adding a sibling action property to the Query type. An alternative, if we don't like this, would be to do something like resource: { action: 'menu/getModules' }

NOTE there's a weird babel error that seems to have been introduced by a dependency upgrade (?) which causes the build to fail (the output in services/data/build/es/index.js is empty when it should contain all the re-exports) so I'm leaving this as a draft for now, but open to API or implementation feedback.

[amcgee]

Related: dhis2/ui-widgets#53

[amcgee]

Implementing an action resource type will avoid this hack which currently (sometimes) works : https://github.com/dhis2/ui-widgets/blob/master/src/HeaderBar/index.js#L30

[varl]

I'm OK with this API, it doesn't squat a key we might want to use in the feature in the query object.

[varl]

NOTE there's a weird babel error that seems to have been introduced by a dependency upgrade (?) which causes the build to fail (the output in services/data/build/es/index.js is empty when it should contain all the re-exports) so I'm leaving this as a draft for now, but open to API or implementation feedback.

It also applies to the CJS build.

Humourously enough, this works:

import { Query } from './components/Query'
import { Provider } from './components/Provider'
import { CustomProvider } from './components/CustomProvider'
import { Context } from './components/Context'
import { useQuery } from './hooks/useQuery'

export { Query }
export { Provider }
export { CustomProvider }
export { Context }
export { useQuery }

---

Edit

Found the issue, fixed in v7.5.2 of @babel/plugin-transform-typescript:

• Re-export statements are omitted from output using @babel/plugin-transform-typescript@7.5.0 babel/babel#10162
• Attempted import error  babel/babel#10156

[varl]

Looks good to me, any second thoughts on this PR or should we go ahead with it?

[varl]

Setting to ready for review as the build issue is resolved.

[Mohammer5]

I don't love this string-parsing API, but it's less disruptive than adding a sibling action property to the Query type. An alternative, if we don't like this, would be to do something like resource: { action: 'menu/getModules' }

If we want to add features like abort/cancel (and possibly other stuff), it'd be confusing to me if we added that on the same level as the "resource" property, so how about this:

const query = {
    apps: {
        resource: ['menu/getModules', { isAction: true }]
    }
}

resource could accept either a string or an array with [ resourceString, config ], so we could add cancellation logic and whatever will come in the future without ever confusing it with request params.
Plus there'd be no string parsing anymore.

[amcgee]

@varl thanks for fixing my silly mistake!

@Mohammer5 That's an interesting idea, I like the cleanliness of the API. I considered something like this as well (also just a string | object type) but I don't think it applies to abort/retry logic for the following reason(s):

---

Let's take abort first. I think the abort needs to be at the query level, not the resource level (a query can include multiple resources). If it's at the resource level, which is what the array-resource method enables, then we somehow have to provide separate abort callback functions for each resource to the user of the query. I think that doesn't make sense, and we need to have a single, global abort callback for each query instead. Something like this:

const query = {
   me: {
      resource: 'me'
   },
   identifiers: {
      resource: 'identifiers'
   }
}
const { error, loading, data, abort } = useDataQuery(query)
....
abort() // This cancels all requests spawned by the query

Note that there's nothing stopping the user from specifying separate queries if they want finer-grained abort control.

---

For something like retry, which would apply only to HTTP errors, I think we want a similar query-level option rather than separate retry behavior at the resource level. Thinking about the long-term API, there's not necessarily a 1-to-1 resource-to-request mapping, since they might be combined or hit the cache, and so the user shouldn't need to specify anything for each resource. I'm not actually sure we want to expose any retry parameters to the user at all, I'd rather the engine just did it automatically.

---

Finally, we get to the one that I think might make sense with a formal parameterized resource model. That's refresh or subscribe functionality, which you could conceivably want to specify at the resource level rather than per query. However, it might still make sense to specify multi-resource queries which have the same refresh or subscribe behavior (or even a first-class useDataSubscription), so I'm still not sure that warrants the parameterized resource

---

Sorry for the long rant, but this is why I avoided making a formal API for this legacy support case. I could still see an argument for formalizing it, but not if we're going to keep it "undocumented".

Here are the options as I see them, very open to votes for what we think makes the most sense as the long-term API:

1. Defer the API decision by introducing this undocumented string-prefix special case
2. Parameterize the resource field with an array or object (resource: [ 'menu/getModules', { isAction: 'value' } ] or resource: { name: 'menu/getModules', isAction: 'value' } or resource: { action: 'menu/getModules' }, leaving resource: 'me' for simple cases.
3. (BREAKING) Move the query-string parameters down one level, maybe underneath the resource key, leaving the top-level query definition namespace available for anything we want to do, i.e.:

const query = {
   idents: {
      resource: 'identifiers',
      args: {
         limit: 10
      }
   },
   // OR MAYBE
   idents: {
      resource: {
         name: 'identifiers',
         limit: 10
      }
   },
   apps: {
      resource: 'menu/getModules'
      isAction: true
   },
   // OR
   apps: {
      action: 'menu/getModules'
   }
}

Are there other options I missed?

This is all a bit out-of-scope for this change which I hoped would be minimally disruptive.

[Mohammer5]

I agree with your concerns!

I think it doesn't really matter what we do to support actions, which we're trying to get rid of anyways, so I wouldn't mind the action::.... resource string.

There's one options, which is a mix of the first and second one:

const query = {
  idents: {
    resource: 'identifiers',
    args: {
      limit: 10
    },
  },
  apps: {
    resource: 'menu/getModules'
    isAction: true
  },
}

To me this seams to be the cleanest solution in terms of api, it allows to fully customize every single resource individually (headers? external? request method?) while making the request args clearly visible

[amcgee]

@Mohammer5 great, yeah I like that one as well and think it's the cleanest. The reason I didn't want to do it here is that it's technically a breaking change, but maybe we're early enough that it's worth it to change all current app-runtime consumers as well? @varl thoughts?

One additional downside is that it makes simple queries a little more complex.

What do you think about the field name args? options might be better? Or parameters or params?

I think it's important to realize that the user shouldn't need to care that this is actually a network request behind the scenes, but rather only care about describing the data they are requesting. Eventually there might not be any direct HTTP requests. As such I think headers and request method (useDataMutation would support different request methods under the hood) should never be added as options here. Similarly, I think external would have to be a different HTTP request hook (perhaps used by useDataQuery, perhaps just with an analogous API) but I don't think useDataQuery should support external data sources.

I realize those were only a few examples you threw out there, but I really haven't seen a compelling use-case yet. It's also important to point out since the args (or options or whatever) field here translates to query string today but should not leak that implementation detail. The limit option, for instance, might be done entirely client-side while the filter option might be passed on to the server (sometimes).

With this design in mind, does options make the most sense?

[amcgee]

Here is a use case for a non-option parameter other than isAction:

const query = {
    idents: {
        resource: 'identifiers',

        // This could be automatically memoized by the engine
        selector: vals => vals.filter(v => v.answer === 42)

        options: {
           // In this case, limit might not be passed on in the query string 
           // since it would limit the output of the selector.
           limit: 10 
        },
    }
}

Looking at this use case, I'm not sure what the distinction is between an option and a top-level arg though - why wouldn't it just be limit: 10 as a sibling to selector?

[Mohammer5]

With this design in mind, does options make the most sense?

I think data makes the most sense (requestBody/body as well, but as you've stated it might not even be a request, so those won't make too much sense here).
options, args, etc. can be misinterpreted

but I really haven't seen a compelling use-case yet.

The indicator's, validationRule's and predictor's endpoint for validating expressions needs a header "Content-Type": "text/plain", it's a post request, so I assume it won't be part of the data provider but a useEffect + useState/useReducer for handling the response?

[varl]

I think pulling out the long-term API design into a separate discussion with executable use cases would be good.

I vote for this to be the scope of this PR:

Defer the API decision by introducing this undocumented string-prefix special case

If we are going to do a breaking API change in the future anyway, then we can just remove this undocumented feature when we do so.

[amcgee]

I think data makes the most sense (requestBody/body as well, but as you've stated it might not even be a request, so those won't make too much sense here).
options, args, etc. can be misinterpreted

I don't think data makes sense for things like limit or filter, though.

The indicator's, validationRule's and predictor's endpoint for validating expressions needs a header "Content-Type": "text/plain", it's a post request, so I assume it won't be part of the data provider but a useEffect + useState/useReducer for handling the response?

On the contrary, I think that absolutely needs to be part of the data provider. But the data provider should do that automatically for expression validation, so the user of the useDataQuery hook shouldn't need to explicitly pass in a content-type or anything else.

Validation might be an edge case here, since it's not really a "data request", so maybe the engine needs a different interface for those kinds of request, but I 100% think they need to be included. An app should never have to make direct HTTP requests to the server if we build this API right.

[amcgee]

I think pulling out the long-term API design into a separate discussion with executable use cases would be good.

I vote for this to be the scope of this PR:

Defer the API decision by introducing this undocumented string-prefix special case

If we are going to do a breaking API change in the future anyway, then we can just remove this undocumented feature when we do so.

@varl agreed

[varl]

@amcgee since everyone agrees that this is an ok solution for now, shall we merge?

[amcgee]

Merged

[dhis2-bot]

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀