Discussion on branching in DCR

branching.md

First, read https://raw.githubusercontent.com/nicl/nicl.github.com/master/_posts/2018-07-12-branching.md.

Now let's take an example from DCR:

https://github.com/guardian/dotcom-rendering/blob/master/src/web/components/MediaMeta.tsx

This file handles media meta for galleries, photos, and videos. It is an example of what the blog post is talking about: it handles different things but doesn't keep branches separate.

To illustrate, look at the following:

export const MediaMeta = ({ mediaType, mediaDuration, pillar }: Props) => (
    <div className={wrapperStyles}>
        <MediaIcon mediaType={mediaType} pillar={pillar} />
        &nbsp;
        {mediaDuration && (
            <MediaDuration mediaDuration={mediaDuration} pillar={pillar} />
        )}
    </div>
);

Some questions/notes:

• which media types support duration? (answer: only one it turns out - video)
• MediaIcon has the same arguments as MediaMeta - a hint that there is not much shared code between media types

Even the icon styles contain a lot of branching:

const iconWrapperStyles = (mediaType: MediaType, pillar: Pillar) => css`
    width: 24px;
    height: 23px;
    /* Below we force the colour to be bright if the pillar is news (because it looks better) */
    background-color: ${pillar === Pillar.News
        ? pillarPalette[pillar].bright
        : pillarPalette[pillar].main};
    border-radius: 50%;
    display: inline-block;

    > svg {
        width: 14px;
        height: auto;
        margin-left: auto;
        margin-right: auto;
        margin-top: 6px;
        display: block;
        transform: ${mediaType === 'Video' ? `translateY(0.0625rem)` : ``};
    }
`;

(Note, the branching on pillar and media type.)

All this deeply nested branching complexes (interweaves) things, which results in brittle code; changing the gallery meta styling, for example, is now a risky exercise, because I need to test and understand the impact on videos and audio as well. For a simple change, rather than reading say 20 lines of code, I have to read and disentangle 100.

It would be better to split this out into three components:

VideoMeta
GalleryMeta
AudioMeta

If it is genuinely worth sharing some of the css, pull it out into a helper file with some exports like:

export const commonIconPadding = ..

Note, it doesn't have arguments for media type or pillar - keep those bits in the specific components to decouple things.

MediaMeta.tsx is really not unusual in DCR - I just picked it because I happened to be working on it. There are lots of other examples!

Finally, why if branching in this way is so bad do we do it?! Well, firstly, not everyone agrees with this analysis. Secondly, people overvalue code reuse and undervalue decoupling. And lastly, often a component begins life in a simpler form and as it develops people are reluctant to refactor into separate components so simply keep overloading the original component.