Passing Config Down With The Reader Monad

[JamieB-gu]

Why are you doing this?

We have functions a long way down our call stack requiring config that's available at the top of our call stack. Currently this config is being passed down via intermediate functions that don't care about it. This is a common programming problem (e.g. in the React world it's called "prop drilling"). The solution is referred to as dependency injection, "injecting" dependencies into the places they're needed without "drilling" them through all the places in between.

This PR accomplishes with the Reader monad, which exists in other languages such as Haskell and Scala (via Cats), and has parallels in React with the Context API (when using the Provider/Consumer pattern).

FYI @frankie297

How does it inject dependencies?

In simplistic terms, in the locations where the config/environment is needed, it can be accessed via the Reader.asks method, which takes as an argument a function of type:

(env: Environment) => ThingUsingTheEnvironment

At the top of the call stack, where the config/environment is available, it is provided to everything below via the .run method, which takes this config/environment and returns the result:

(env: Environment) => Result

What else can it do?

Because the Reader is a monad, it also has the andThen and map methods available (like the Option and Result types). These can be used respectively to chain multiple Readers together, and to transform the values they hold.

Changes

• Created the Reader in the Reader.ts file.
• Moved isImage from server.ts to the image.ts module.
• Pulled the unpacking of fields from the CAPI response into the Article and LiveblogArticle components.
• Made use of the Reader to pass imageSalt down to components that need it (for generating image URLs).
• Tweaked the HTML generation pipeline in server.ts.

Future Work

• The code in server.ts used to parse the CAPI response and generate the HTML has become unpleasant. I have a plan to fix this in a future PR, which may also make the Reader.sequence code nicer.
• Possibly improve the type definition for Reader to make it less verbose.
• Improve on the Reader.sequence solution (sequence shouldn't technically live on Reader, it belongs on Traversable).

[alexduf]

I think I'm warming up to the Reader monad, it might come handy when we have more dependencies to inject.

However it seems to break the JSX model. You can't call your component directly within a JSX snippet anymore without mapping or sequencing over the reader. I think this kind of negate the benefits of JSX. I know you already feel strongly about JSX and how we should only call functions, but react without JSX isn't quite the same entry level.

We talked about how we might structure the project in three steps last time:

1. Fetch the data
2. Transform the data
3. Render the page

I'm wondering if we shouldn't use the Reader monad for steps 1 and 2 only, and ensure at the time of rendering we already have all the data, so no need for dependency injection.

How would that feel @JamieB-gu @webb04 ?

[alexduf]

I'm wondering if this isn't a smell. In scala it would have been trivial to have a for comprehension to directly work with the content of these, but here it feels a bit convoluted.

I had to read back and forth quite a bit to understand that HeadeImage(and articlebyline etc) was a function that returned a reader, and that was why you need to sequence them.

[JamieB-gu]

I'm in two minds about this. I do have a plan to make this work more like a for comprehension; that's what I was alluding to with "I have a plan to fix this in a future PR, which may also make the Reader.sequence code nicer".

However, this use of sequence is actually a more conventional JavaScript pattern, as it mirrors Promise.all, (which is more or less just sequence, although sequence should really live on Array rather than Promise...but JavaScript). I thought about adding a method alias to Reader called .all to make it look more like Promise.

[alexduf]

That's a good shout, it's not that different!
It still breaks JSX, so I guess sequence aside my points above might still stand?

[JamieB-gu]

That's a good shout, it's not that different!

Would you prefer a method called .all to mirror what Promise is doing? Would that make it more readable (also a question for @webb04)?

It still breaks JSX, so I guess sequence aside my points above might still stand?

See my (overly long 🙂) reply on the JSX comment below.

[alexduf]

Would you prefer a method called .all to mirror what Promise is doing? Would that make it more readable (also a question for @webb04)?

I guess not, but it might be worth popping up a comment above the sequence definition with the analogy.

[JamieB-gu]

Ah! You've just reminded me, I meant to put JSDoc comments alongside the methods. VSCode picks those up via Intellisense; unsure about IntelliJ but it may do too.

Will do that in a follow-up PR.

[JamieB-gu]

You can't call your component directly within a JSX snippet anymore without mapping or sequencing over the reader.

Hmm, I don't personally have a problem with doing this, as I've seen a fair bit of JSX code that's more "logic-heavy" than this before now. However, if I understand your point correctly your concern is readability and accessibility for new developers?

Is it specifically the fact that these components are now being called with { component } syntax rather than <Component propOne="foo" /> syntax? One solution for that might be something like this:

const Component = (props: { aNumber: number }) =>
  getSubComponent().map(SubComponent =>
    <div>
      <p>Some words.</p>
      <SubComponent propOne={prop.aNumber} />
      <button>Click me!</button>
    </div>
  );

Or is it specifically that we're returning JSX within a callback? If that's the case, would something that looks like this be preferable?

const Component = (props: { aNumber: number }) => {

  const SubComponent = getSubComponent();

  return (
    <div>
      <p>Some words.</p>
      <SubComponent propOne={prop.aNumber} />
      <button>Click me!</button>
    </div>
  );

}

If the second would solve the problem, that may be something that's possible with the for comprehension-style solution that I'm working on now.

Would you consider this a problem regardless of whether we were using JSX or not, or is it specifically a problem with how this interacts with JSX syntax?

[alexduf]

My concern was the first one, and I didn't realise it was possible to keep the syntax the same. What impact (if any) would that have on the declaration of the component?

[JamieB-gu]

What impact (if any) would that have on the declaration of the component?

Ok, I've been experimenting and I think it would be possible to do this and make the component declaration reasonably nice. However it will probably require the for comprehension style construct I mentioned above for the types to line up when unpacking multiple Readers.

I'll try to explain why I think that's the case; sorry in advance for how long this upcoming comment is.

The problem is that the type of sequence is:

<E, A>(rs: Reader<E, A>[]): Reader<E, A[]>

which means that the type of each item in the array must be the same type A.

If we implement the change described above, the type of getComponent is:

const getComponent: Reader<Env, React.FC<ComponentProps>>

If you're sequencing an array of these, the types may be something like:

type ArrayOfComponents = [Reader<Env, React.FC<ComponentOneProps>>, Reader<Env, React.FC<ComponentTwoProps>>, Reader<Env, React.FC<ComponentThreeProps>>]

Here the generic A mentioned above should correspond to the React.FC<...> types, but it doesn't because these types are different from one another. The props used to parameterise them are different:

ComponentOneProps ≠ ComponentTwoProps ≠ ComponentThreeProps

∴ React.FC<ComponentOneProps> ≠ React.FC<ComponentTwoProps> ≠ React.FC<ComponentThreeProps>

∴ ArrayOfComponents ≠ Reader<E, A>[]

(Sorry for dropping into pseudo-mathematical notation, I was hoping it would illustrate the problem better than words)

The upshot of this is that I think unwrapping multiple Reader should be possible with a for-comprehension-style structure, because that will allow this "A" type to be different for different Readers. This is demonstrated by the fact that I believe you can do this in Scala:

val optionOne: Option<String> = Some("foo")
val optionTwo: Option<Int> = Some(42)

for {
  a <- optionOne
  b <- optionTwo
} yield a ++ b.toString

[webb04]

It's a valid warning but should we disable it?

[JamieB-gu]

I think React may throw a warning for this at runtime, so yeah maybe we don't need the lint rule. Also, I don't think it would matter on the server because isn't it a performance optimisation for re-renders?

[JamieB-gu]

We're not going to implement this due to potential complexity. Instead I'm going to close it and treat it as a spike.