Generate additional Elm endpoints that use Task

[micahhahn]

Context

In content creation, we have many cases where we need to issue lots of requests to the backend. This is painful using our typical generated endpoints because you need to create a Msg constructor for each one to manage the handoffs.

A much better solution (🎩 @brian-carroll ) is to use Http.task which returns a Task instead of requiring a Result err ok -> msg argument. Tasks can be chained together with andThen and are much more flexible in generate as you can convert a Task to a Cmd Msg, but not vice versa.

Solution

This library generates an extra set of endpoints that return Task. In fact, we redefine the Cmd msg endpoints to call the new Task endpoints. The original endpoints maintain the same name / argument order to make upgrading the monorepo easier.

Reviewing

You can see what these changes look like in the diffs of the golden files of this PR.

I also have a PR up in the monolith (https://github.com/NoRedInk/NoRedInk/pull/47150) that is targeting this branch if you would like to see the effect merging this to master would have on the monorepo.

[nunocf]

Looks great!

[brian-carroll]

Good stuff!
I was trying to think of why we currently use Cmd.

I think a possible advantage is testability with elm-program-test. If you have one message per HTTP response then it's very easy to write the test code and keep things maintainable. If your effects involve a complex chain of tasks, I can imagine it gets harder to build the mock version, and keep it maintained and correct.

@NoRedInk/sdgg might have some insights.

You mention "upgrading the monorepo" but Task may not be strictly better in all cases. In the vast majority of cases it probably makes no difference to the application, so if it leads to less testable patterns in our code then it is not an upgrade.

We have a fairly uncommon case in the admin tools that Phoenix is building. We are chaining together a bunch of HTTP calls that are originally designed to be called one at a time for "normal" non-admin use. So we have a good reason to chain calls together rather than merge them into one. But normally there is no good reason for that. If we were doing this kind of chaining in user-facing pages I would say it was likely an architecture mistake.

So I think it's great to have the Task versions for cases where we need it, but I'm not convinced that we should migrate everything to use it.

[brian-carroll]

Currently the only reviewers on this are Phoenix members but this library is used by all Eng teams. I'm adding SDGG.

EDIT: they don't have a team alias that I can assign! OK the mention in the comment above will do.

[juanedi]

I was trying to think of why we currently use Cmd.

I don't think we have any record of the reasoning behind that decision, and can't think of anyone still here that might know

I think a possible advantage is testability with elm-program-test. If you have one message per HTTP response then it's very easy to write the test code and keep things maintainable. If your effects involve a complex chain of tasks, I can imagine it gets harder to build the mock version, and keep it maintained and correct.

I think a use case like yours might require some extra work to maintain the level of testability, but it doesn't have to be a deal breaker!

elm-program-test's API for dealing with Http should play well with Tasks. The library has its own SimulatedTask type, which has similar combinator functions as Task (docs).

So... just like perform will know how to turn a single Effect value into multiple Tasks that it knows how to combine, we can probably mirror that logic for the SimulatedTasks when simulating effects.

If we locate the code to simulate the effects inside the Main module rather than in the test (which I think is a good practice anyways), we can probably find ways to minimize the code duplication and the test pretty robust.

@brian-carroll do you think that would address your concerns re. testability?

Really excited to see how this ends up looking! Leaving a stamp for now because I think this change makes a lot of sense 😻

[juanedi]

Oh, in case it wasn't clear from my comment above: I couldn't come up with any other potential downsides of this approach 😅

[micahhahn]

upgrading the monorepo

@brian-carroll By this I just mean upgrading to the new version of servant-elm - not switching all the monorepo endpoints to the Task version!

Thanks for the good thoughts on testability. We also generate SimulatedTask versions of the endpoints, I'd love to see how the testability compares in practice.