feat: Add Option and Result

[SiAdcock]

What does this change?

Migrates the Option and Result types and associated logic from @guardian/types to @guardian/libs

Why?

See guardian/types#136

TODO

• write tests for Option
• Better READMEs – extract from JSDoc??
• Is this the right location for types? Or should we move to the /types folder?

[coveralls]

Coverage decreased (-5.4%) to 90.698% when pulling 4c29359 on sa-option-result into 0c28cfb on main.

[SiAdcock]

@JamieB-gu I might need your help writing a readme for these modules... do you have time some time? 🙏

[JamieB-gu]

Sure, happy to! How would you like to do this? I can push to this PR, I can create a PR into this PR, or we can pair?

[SiAdcock]

Before I go much further with this migration, I'd like to question whether this is a good addition to guardian/libs.

I'm going to make the case that code in this repo should represent The Guardian P&E's recommended way of doing things. Having a repo that is the go-to starting point for solving common problems is extremely valuable for onboarding, and for delivering new products and features quickly.

Having a single recommended approach to solving common problems, where possible, is more valuable than having multiple options to choose from. Offering multiple solutions for a single problem introduces more work for consumers who must evaluate each of those solutions. This is inefficient, and inefficiency comes with an opportunity cost.

There will always be trade-offs between options and those trade-offs should be made clear. There is nothing wrong with using a non-recommended approach if the trade-offs of the recommended approach are too great.

Does this sound convincing / useful? I'd love to hear your thoughts

[SiAdcock]

Assuming you are convinced and you now believe that this repo should contain The Guardian P&E's recommended way of doing things, we can evaluate whether the Option type ought to be the way we represent things that may or may not exist in TypeScript.

Option

Option represents a value that may (Some<A>) or may not (None) exist. None provides a wrapper around null or undefined. Some provides a wrapper around a defined value.

I must admit I'm not used to working with types like this, so I'm struggling with the way it is being presented. I'm going to attempt to unpack what the API is doing to help my understanding. If I have anything wrong, please let me know.

withDefault

 const bylineOne = some('CP Scott');
withDefault('Jane Smith')(bylineOne); // Returns 'CP Scott'

const bylineTwo = none;
withDefault('Jane Smith')(bylineTwo); // Returns 'Jane Smith'

In TypeScript, we would do something like:

 const byline = 'CP Scott' || 'Jane Smith'

map

const creditOne = some('Nicéphore Niépce'); 
map(name => `Photograph: ${name}`)(creditOne);  // Returns Some('Photograph: Nicéphore Niépce') 

const creditTwo = none;
map(name => `Photograph: ${name}`)(creditTwo);  // Returns None

In TypeScript, we would do something like:

const result = credit ? `Photograph: ${credit}` : null

In the interest of brevity, I shan't continue. From my partial analysis, it appears the Option API is an attempt to avoid explicitly dealing with null or undefined values, and further to avoid using constructs such as nullable types (e.g. type MaybeUser = User | null), optional chaining, the ternary operator or the || and && shortcuts for non-null assignment.

I'm trying to understand the total value that this API provides, and if it's doing something that can't be done in the ways I've described above. From what I understand, its total value is that it provides a more readable API for devs who come from a functional programming background? Would this be fair to say?

I'm interested in @JamieB-gu's thoughts, but these questions are open to anybody and everybody. I'd like to understand whether we should be recommending that everyone in the department start using this approach.

[JamieB-gu]

Would you like me to write some tests for Option too to keep coveralls happy?

[SiAdcock]

Thanks for offering, I'd say maybe in the future. But I think before we commit any more time to this, it would be useful to address some of the points in my earlier comments. Would you be able to take a look?

[JamieB-gu]

Was in the middle of writing when you commented this 🙂. See the essay I shared below 🙄.

[JamieB-gu]

Thanks for doing this @SiAdcock! So quick too 🚀. I appreciate you trying to gather some feedback on the API - I'll try to do my best to describe the thinking behind it below. Apologies for the essay 😬.

• Better READMEs – extract from JSDoc??

Yeah I think the API docs can mostly be copy-pasted from JSDoc. The JSDocs are there mainly for Intellisense convenience (they pop up whenever you hover over the types/functions from this lib).

• Is this the right location for types? Or should we move to the /types folder?

I don't think they need to live in their own isolated directory, but I don't feel too strongly. It's not going to make much difference to consumers 🤷‍♂️.

I'm going to make the case that code in this repo should represent The Guardian P&E's recommended way of doing things.

Fair point, I think that's valuable too. I guess the counter-argument to that is that there isn't always a one-size-fits all solution, and some parts of this library will be useful to a subset of projects in the department.

For example, the logger at the moment looks targeted to a specific problem on Dotcom (multiple teams contributing to one platform, the window.guardian object, and the dotcom-specific dev domains). Perhaps it would be useful on other platforms too (e.g. Apps/Editions), but we'd probably have to adapt it for that?

Likewise the storage module. That's written in an object-oriented style that's quite different from the code in Apps/Editions-rendering (and actually I think it's the only use of classes in the libs repo from what I can tell?). I'm obviously not saying there's anything wrong with that - I think my point is that there's some variance in approach within this repo already, and some areas of it that are useful to some platforms, but perhaps not all, in their current form?

Perhaps. Or perhaps that's fine, and the problem is that we haven't proliferated the modules in this repo across enough of our projects yet and we should be focusing on that? What do you think?

Option represents a value that may (Some<A>) or may not (None) exist. None provides a wrapper around null or undefined. Some provides a wrapper around a defined value.

I must admit I'm not used to working with types like this, so I'm struggling with the way it is being presented. I'm going to attempt to unpack what the API is doing to help my understanding. If I have anything wrong, please let me know.

Based on what you've said and the examples I think your understanding matches the intent 🙂. Only thing I'd mention is that None doesn't wrap null or undefined. Option is just a type and Some and None are just JS objects; they don't say anything about null or undefined one way or the other. But you can create an Option from a value that may be null or undefined using the handy fromNullable function.

an attempt to avoid explicitly dealing with null or undefined values

to avoid using constructs such as nullable types (e.g. type MaybeUser = User | null), optional chaining, the ternary operator or the || and && shortcuts for non-null assignment.

From what I understand, its total value is that it provides a more readable API for devs who come from a functional programming background?

I'd argue that it provides a more readable API for new developers coming from a number of languages, not just functional ones; for example, Scala, Rust and Swift all have concepts of Option and none of them are particularly functional. But I'd say that's a side benefit (and the reason I named it Option rather than Maybe), not the main one.

Its main value is actually what you mentioned in the first two sentences above. It avoids dealing with null/undefined and the many gotchas that come with using them and JavaScript's type coercion, which will often confuse and catch out new-comers and less experienced developers (and plenty of "experienced" developers 🙈). I'll try to use the two examples you gave above to demonstrate what I mean. Apologies for the over-the-top story-telling style 😬.

const name: string | null | undefined = 'CP Scott'
const byline = name || 'Jane Smith'

There's a fair bit of complexity hidden in this pattern for assigning defaults. First off, at first glance surely this doesn't make sense? You've got a boolean operator, but neither side is working with boolean values? Oh wait, it turns out that JavaScript will implicitly convert these values to booleans under the hood. Ok... So what is the boolean equivalent of a string? Right it's true. And the boolean equivalent of null? false? undefined? Also false. What's the difference between null and undefined and why do we have both? Do they always behave the same...? Ok, not going to worry about that now, gotta get this working. Anyway, great, so whenever I've got a string this will return true. Oh wait... I just passed an empty string '' and it came out false... So strings are always true, except when they're empty, and then they're false - ok I guess I can remember that... But the important thing is that this is a boolean operation and so the resulting byline will be a boolean! Sorted... Wait, what? It came back as a string...?

const credit: string | null | undefined = 'CP Scott'
const result = credit ? `Photograph: ${credit}` : null

I won't go through the whole self-indulgent story again for this one, but I think it suffers from many of the same problems. Boolean type coercions, empty strings, null vs undefined etc. There are also some new issues: the implicit coercion of something that's not always a string for use in a template literal for example, which typescript-eslint will likely complain about (actually, it probably won't in this case because of another implicit conversion to boolean before the ? which acts as a type guard 👀 for the compiler).

The point is, I don't disagree that these are common patterns in JavaScript. My argument is they're common patterns with a lot of problems and that I think we can do better. Particularly if, as you mentioned, we'd like to make it easier for less experienced/junior developers to contribute.

[SiAdcock]

@JamieB-gu Thanks for responding, these are really great arguments.

there isn't always a one-size-fits all solution

I agree. Trade-offs of any recommendations (and alternative solutions if the trade-offs are unbearable) should be made clear, so that people can make the right decision for their use case. create-react-app does a fantastic job of demonstrating what its strengths are, and which alternatives you should consider. I also agree that some existing libs in here need better documentation or extension to achieve this goal.

If we're going to provide recommendations, we should by default recommend using the language to solve the problem. This means we have something to fall back on, that we don't have to provide specific recommendations for every line of code that needs to be written. It also helps to ensure that we're always using the right language for the job, we're not just picking up the familiar tool.

What we're boiling down to is whether TypeScript (and really, the JavaScript syntax that it has inherited) is adequate enough at communicating the intention of developers using it. In what situations are we not convinced that it is communicating clearly enough?

Where the language falls short in some way, or where it isn't designed to deliver a particular feature, then we can provide recommendations for those scenarios. And I think Option falls into this category. The hypothesis is that TypeScript (and JavaScript's) type coercion, the concept of falsy, and the common patterns that have emerged from this hide a lot of complexity, are confusing for developers and slow down feature delivery. Falsy has some particularly fiendish gotchas that can cause frustrating bugs that make eyes roll. I know I have fallen victim to the falsy empty string problem.

I've also used these patterns correctly hundreds of times, and there's no question that this style of programming can be fast to read, write and review, and delivers (mostly) adequate results. Reading the relatively short JavaScript: The Good Parts will get new developers up to speed, and there may be better resources out there today (please ping me if you know of any).

Personally I'd like to see more discovery work in this area before we try to patch over this particular language feature. Are new devs finding type coercion a source of confusion? Are we getting tripped up time and again by falsy? Is the complexity slowing down reviews, or introducing corner cases that let bugs slip through the cracks? It would be useful to see some examples or track down data that shows where the Option approach would have helped, so we can get a sense of how widespread the problem is.

From the other end, we ought to consider the opportunity cost of introducing this approach. A case could be made that the approach is unfamiliar to client side devs, existing or future, from associate to senior. There is a ramp-up cost to them learning this, which could otherwise be spent solving problems for the business or reducing technical debt. It would be useful to understand how quickly we can get unfamiliar devs up to speed with Option. Do devs using this approach enjoy using it? Would they recommend it to others? Will they use it again in the future? In which scenarios does it works best?

This is my 10 pennies' worth, I'm still up for reading yours and other people's opinions. But really I think we need to demonstrate the business case for this approach, if we are seriously going to consider recommending its use across the department. Please let me know if I can help with this in any way.

[JamieB-gu]

That all sounds very sensible to me - I like the approach you're taking to this ⭐️. I think I've mostly made my case so I won't give other people reading this yet another essay to wade through 🙈. I'll just address a couple of specific points you made.

Are new devs finding type coercion a source of confusion? Are we getting tripped up time and again by falsy? Is the complexity slowing down reviews, or introducing corner cases that let bugs slip through the cracks?

I've seen many examples of both confusion/review overhead and bugs introduced over the years. For example, this PR yesterday fixed a subtle bug caused by null/undefined, which existed despite the new optional chaining syntax introduced to JS to make them easier to deal with.

A case could be made that the approach is unfamiliar to client side devs, existing or future, from associate to senior. There is a ramp-up cost to them learning this, which could otherwise be spent solving problems for the business or reducing technical debt. It would be useful to understand how quickly we can get unfamiliar devs up to speed with Option.

I think another way to look at this would be to argue that you actually save time in the long run. That's if you assume that, as a software developer, JavaScript won't be the only language you use in your career. Admittedly for some people it may be, but many are either coming to it from another language, or would like to experience working in other languages in the future (we developers are a naturally curious group of people 🙂). For example, just within the department it's common to be expected to work in at least two languages: TypeScript and Scala (two and a half if you consider that JS and TS can look quite different).

If you accept that premise, then I think it's valuable to spend your time learning concepts that are common across different languages and environments. Speaking from my own experience, I originally learnt some of these concepts from Elm and Haskell, and was then able to trivially extend that knowledge to understand them in Scala, Rust, Swift, TypeScript and so on. I see the alternative as spending your time learning the specific quirks and patterns of a language that's notorious for having a lot of quirks, and then being able to re-use very little of that knowledge in other environments and languages.

[SiAdcock]

Today I remembered that there were some interesting takes when we had this same discussion a year ago in guardian/types#5

[nicl]

As @SiAdcock suggests earlier, union types and the ternary are idiomatic, not verbose, and work well. As much as I like Option etc. in Scala, I'm not convinced this is a problem in TS that needs solving. The benefits will be less too than for Scala etc, because the pattern is not widespread - e.g. there are more 'edges' to coerce into the pattern than in Scala etc where at least libraries will already be using wrapper types like this - and because of the lack of equivalent pattern matching, which hampers readability when branching.

[sndrs]

I'd argue that it provides a more readable API for new developers coming from a number of languages, not just functional ones; for example, Scala, Rust and Swift all have concepts of Option and none of them are particularly functional. But I'd say that's a side benefit (and the reason I named it Option rather than Maybe), not the main one.

many are either coming to it from another language, or would like to experience working in other languages in the future

i think we should be optimising our code for developers who know the language it's written in before ones who don't. and further, i'd argue familiarity with the language of the code and its common practices ought to be sufficient: the less you need to know to be useful, the more you can do.

bespoke interventions like these have both financial and opportunity costs (you have to pay someone to learn an unfamiliar idiom, and that time is then not spent doing something else).

so then the question for me is: can the problem this addresses be handled without paying that cost?

if there is a provable case for the benefit here (or even majority support) then i would say something like fp-ts is more scalable than a custom implementation – you get docs, support, wider adoption etc (there's a lot of value in knowing what we need to know to work on something).

but until then, since this is implemented in TS, it means it's possible to achieve the same outcome in 'native' TS. which means this is more a question of taste (both this and a more imperative, idiomatic approach work).

imo, such questions should defer to our (current and future developers) ability to write, ship, fix and delete code i.e. build products.

to me, this feels like it prioritises DX (for some people) over the ability of people who know TS (from a little to inside out) but have never seen this model to contribute.

this is not to say that js (and so ts) doesn't come with footguns, but i'd argue that having the computer point out where you've not handled one (tsc) is the 80% solution (it's typescript's "overwhelming adequacy" as @AWare put it so amazingly).

having to grok a new mental model in order to elegantly skirt round a shortcoming is the 20%, but it comes at the cost to the 80%...

[ashishpuliyel]

This is a very thoughtful tread, and I appreciate how carefully and clearly people have put their thoughts together. I'm reading it today for the first time, and I'm loath to say anything because I fear I've not yet put in as much thought as you have. But I will anyway.

I'm definitely more client-side (and a lot of my server-side experience was JS too), and much more comfortable in TS than I am in Scala, so my instinct is not to like this.

I personally think we should write each language following the standard patterns and best practices of that language, and that makes for a codebase that's easier to maintain and especially for new developers to learn. As always we should be writing code that's easy to read (as opposed to easy to write). If someone comes in in knowing TS they should be able to read and understand how the TS code works. The code shouldn't do things that are easy to do in TS in unusual ways. Using Simon's examples of what I'd consider idiomatic TS, I really prefer them (with their known, well understood and documented limitations) over this specific abstraction.

There is no doubt learning new languages has enriched and improved my ability to write code. And we should be encouraging TS devs to pick up scala. But I'm not sure that means I should be writing TS like it's scala.

[sndrs]

After a lot of consideration, we have formalised and generalised the conclusions of the discussions around this PR in https://github.com/guardian/recommendations/blob/master/coding-with-empathy.md.

Our conclusion is this is not a pattern we want to formally adopt in our TS codebases, because it currently diverges too far from we might reasonably expect a TS developer to know.

That being said, there is clear support for something along these lines.

Hopefully a similar pattern will make its way into TS itself (can we PR this upstream?).

[JamieB-gu]

Fair enough. We may have to rethink deprecating @guardian/types if this is the case, because doing that was predicated on the idea that we'd migrate its content to @guardian/libs 🤔.

[SiAdcock]

Please can you expand on that a little @JamieB-gu? What would be the impact of deprecating "guardian/types from your / your team's perspective?

[JamieB-gu]

Well based on @sndrs summary above, we've decided not to migrate Option and Result to libs. I think this is fair enough if we feel they don't belong in this repo - I mean even Format and Role have needed to be adapted to fit into this context.

That said, in order to deprecate @guardian/types we planned to move everything across to libs. If there's some stuff that can't be migrated then it'll have to stay where it is so it can be consumed by the projects that use it, receive updates and bug fixes etc.. Therefore we can't deprecate @guardian/types 😬.

[sndrs]

i think it's only apps-rendering and image-rendering that use them – can they be inlined there? do they need to be in a library?

also, should it really be used in an library like image-rendering? if this is intended to be a cross-platform image component, i'm not sure it should be depending on it...

[SiAdcock]

should it really be used in an library like image-rendering

Given the conclusions above, I'm inclined to think not. My main concern here is that Option<T> is part of the public API, which forces codebases to depend on Option in order to consume image-rendering. Have I summarised this correctly?

[JamieB-gu]

i think it's only apps-rendering and image-rendering that use them

They're also used in mobile-purchases.

can they be inlined there? do they need to be in a library?

I think, given they're used in at least three different projects, a library is the best place for them. I don't think it's ideal to make several PRs for every bugfix or new feature - that's kind of the point of a library 🙂. As it is there are a couple of new features implemented in AR that should really live in the library - they were just there for now while we were waiting for the migration.

also, should it really be used in an library like image-rendering?

Given the conclusions above, I'm inclined to think not. My main concern here is that Option<T> is part of the public API, which forces codebases to depend on Option in order to consume image-rendering

Yeah I think this is reasonable. In this case I don't feel the API is too onerous for the consumer: it's just a case of using the some function or none value. However, I think it's a fair point that, in general, library A shouldn't ask you to also rely on library B in order to use it (except something that's explicitly an add-on, e.g. @emotion/react).

So, in summary, we can remove them from the public API if you think that's best 👍. Note that this won't stop image-rendering depending on @guardian/types though, as it still uses them internally...