new version of http that uses fetch

[taylordowns2000]

build a new fetch-based version:

1. round up all outstanding issues related to http, take them into consideration
2. make sure the new version adheres to the template style
3. take into consideration the expandReferences dilemma
4. confirm that we should drop the common.http since all users will now have access to fetch directly

[josephjclark]

There are a few things going on at the same time here

1. Replace axios with unidici (fetch won't work). This is a non-trivial and breaking change (also language-http is hugely affected)
2. Review the common http API signatures, and look at how they can be re-used across adaptors.
3. Review the new pattern of adaptors providing their own fetch-based http adaptor in a utils file.

Each is a big and breaking update, but it might make sense to consider all of them together.

[josephjclark]

This is the expandReferences dilemma:

expandReferences

expand references find what I call a "deferred value" - a value nested inside a function which must be resolved against the latest state.

expandReferences(arg) will recursively find any functions on arg and invoke them with state. It will do this for all members of an array, all keys of an object.

So this pattern, with two nested deferred values, should be totally fine:

get(url, { query: (s) => s.q, headers: () => s.h })

This pattern defers the whole query object, which is also fine (and for my money, better)

get(url, (s) => ({ query: s.q, headers: s.h }))

I do not know why this recursion is needed. Nesting deferred values doesn't do anything but complicate the code - you don't get a different value for state or anything. I actually think it's an antipattern.

Basically every adaptor supports/depends on this recursive expansion. If we change it, we break every single adaptor in a really subtle way.

expandRequestReferences

The common.http functions expose all the parameters of an axios request to the params argument

Many of axios's options look like deferred values to our recursive expander, so we'll try and invoke them. And almost certainly break something.

So expandRequestReferences was introduced to enable safe recursion over axios' options by only trying to expand a small number of keys.

It looks inelegant from the outside, but it's a good and necessary solution.

[josephjclark]

Replacing axios

Remove the dependency on axios in language-common and language-http and use unidici instead.

Axios and unidici do not share an API. And unfortunately common exposes axios's options directly to consumers.

Also remember that common exposes the axios object directly. Jobs which depend on this will be incompatible with this version, and authors writing new jobs against the new adaptor may need to retrain themselves.

We had been looking at using node:fetch instead as a "standard" solution. But there are two compelling reasons to use unidici:

1. We need support to pass a custom http agent, which fetch does not allow (but unidici does)
2. We are likely to to use unidici's excellent mocking API in our unit tests

Options:

1. Go ahead and break the API. Implement a subset of the unidici API, and do it in a way that we think is easy for our users.
2. Continue to use the axios API, but map it (as much as possible) to unidici. We probably still have to break the axios export, so this is still a breaking change.
3. Continue to use axios and forget all about this.
4. Continue to use axios but introduce a new API and add deprecation notices to the old one.

Whatever we do, we should continue to expand the unidici arguments like usual, which basically means keeping the expandRequestReferences approach. We may want to change this later.

[josephjclark]

Breaking Changes

What do I mean when I say something will break? That's not always obvious for us

Existing jobs on platform will continue to run because they're version locked to an adaptor. And old adaptors will not be affected by anything we do (if we change common, and an old adaptor depends on common, it'll install within the major version).

When we migrate platform jobs into Lightning, they should continue to run so long as we continue to lock versions. That's an important note for the migration strategy - we should NOT migrate and upgrade.

So the only people affected by breakges are people writing new jobs, which I think is mostly our internal team.

There are two things I'm concerned about:

1. A major version bump to common probably means a major version bump to every adaptor (especially if we change eg references). And some of those adaptors will need to change. Which means a) a very large testing surface and b) confusion and uncertainty when working with adaptors (it is not nice to be mindful of two different APIs for an adaptor and to have to check your version all the time)

2. Changing the patterns and semantics we use when job writing will upset muscle memory. An experienced job writer may have to adjust their patterns when working on new jobs, and in subtle ways. This isn't easy! Imagine if we renamed fn to fnc or something - how long would it Taylor to retrain himself to use the new format?

If we're going to break something, it really ought to have meaningful impact and benefit.

It's often a good idea to add a totally clean new API, rather than tweaking or adjusting an old one. Benefits of this are:

• You have the option of leaving the old API in the code for one major version (probably with deprecation logs)
• Confusion is decreased: you can continue to use the old ways, or you can switch over to the new ways. Either way your mind is in the right place, and you're less likely to be caught out by a subtle change.
• Documentation only documents the new approach (if the old apis are still documented, they are clearly marked as deprecated)

[josephjclark]

Removing common.http

Do we even want to keep common.http?

We've started developing a pattern where each adaptor provides its own http helper functions using request.

This is based on the idea that adaptors can just use the native request handler, a simple and convenient low-level API that we can't improve.

This gets a bit less clean if we switch to undici, and it's not particularly nice for our documentation to just link to unidici's docs (I actually hate it when people do this, I hate going to node.js docs for fetch and it links to MDN).

Watching this approach get rolled out across a few adaptors, I can also see that it's generating a lot of boilerplate code. It's becoming a barrier to template creation - the first thing you have to do is write a bunch of low level http handlers. I think we're actually making life harder.

A final note on this is that this pattern discourages uniformity, which may be bad for job authors. If adaptor A and adaptor B take very different approaches to http handling, the display (or not) of http errors in the log may very in quality, location and content.

Isn't it better for us to abstract a bunch of this out, to use our expertise to write a really tight pattern for http, and just give template authors clean hooks to use it? Better devx for adaptor authors and job writers.

I think yes, we should remove common.http, but we should replace it with a bunch of non-operation, non-expanding helper functions which wrap unidici in common.util. These are designed to be used specifically by adaptor writers and not to be exposed to job authors (if you want to make a http get call, use the language-http adaptor!).

[josephjclark]

Proposal

Ok, having been through a bunch of this, here's a proposal for what to do about http.

This is a huge project, a lot of work, with some design needed:

Guiding principles

• We will ditch axios and standardise on unidici
• This will break everything and require major version bumps across the adaptors (frankly this is unavoidable)
• Almost every change - the ones that matter - will be in new APIs, to reduce confusion
• We have the option to deprecate existing APIs (and I suggest we do this, but I'm open to being more aggressive)
• References are basically untouched

Add http helpers to common/util #314

We write a new layer of http functions in common/util, designed to be used in other adaptors.

The new functions are not operations or operation factories. They do not take and return state. They do not expand references.

We present get/put/post etc functions which wrap unidici. Each is a promise of the form get(url, options). We present our own API as a subset of unidici, and if people want more functionality we'll expose it (at least this way we have some idea of what people actually use).

Options allows you to pass query arguments and headers and stuff. It will also allow you to map error bespoke messages to error codes via simple mapping, ie { 401: "Failed to log in to dhis2" }.

I'd also like to design a url builder function to make URL construction easier.

The full signatures here are to be decided.

The handlers might be stateful - perhaps optionally. This would allow an adaptor to set auth and agent and maybe error messages at the start of execute, and reduce repetition thereafter.

Remove common.http #315

common.http is for the axe. The whole thing should go.

It will be replaced by the new helpers in common.util.

We could deprecate it for one major version. This would allow us to phase the work more effectively and be a little bit more gentle on job authors.

Rewrite language-http #316

Probably the biggest change is language-http - it needs rewriting and redocumenting to the new format.

I would also like to update http to use the new format of expand references, without any of the skip stuff. I don;t think we need a confusing expandRequestReferences function: either we ensure that the options include only expandable values, or we add support for streams to the new expandReferences, or we slightly rethink how the keys are split and passeD to expandReferences.

I basically want a license to remove the skip flag from the new expandReferences, and I want it to be a little bit clearer why http has special handling (or even better, no special handling at all!)

Update the template

Adaptors should use get handlers in common/util, not use unidici directly.

I think this removes the need to create custom handlers for each adaptor in a Util file, simplifying the code.

The template should be updated (again) to reflect this. We should go over each existing adaptor which uses the pattern and make sure it's up to date.

expandReferences

I don't want to touch it.

I think this will just introduce subtle bugs for no good reason.

We should continue to support the old, recursive function.

The language-http rewrite should simplify the expandRequestReferences stuff.

Note that, while we're introducing a major change, we should update every adaptor to use the new expandReferences function, and deprecate or remove the old one (there are issues for this).

Maybe we should check that the documentation recommends you don't rely on recursion- or indeed doesn't tell you you CAN recurse. Pushing away from that pattern now may enable us to change it later, if there's a compelling reason to do so.

Other adaptors #317

We should go through each adaptor, remove axios and common.http re-exports, update expandReferences, remove otherhttp libraries (ie superagent) and bump the major version.

[josephjclark]

Closing in favour of epic #320 and the sundry issues under it