[RFC] RenderingTarget type and varying props by platform(?)

[OllysCoding]

Note: In earlier versions of this RFC, RenderingTarget was called Platform, however it's been rightfully pointed out that Platform as a term is overloaded, so hopefully RenderingTarget better reflects what we're trying to do here!

Background

DCR is being extended to support rendering for both web & apps, and as a result AR will eventually be decommissioned.

Content pages (articles) differ slightly between web & apps, and DCR must now be updated to support these differences, these include:

• Different reader revenue propositions
• Different ad stacks
• Extra content, like follow buttons for apps
• Web integrations being replaced with native integrations (e.g video, lightbox)

For this to work then, our components in DCR will need to be aware of what they're rendering for, and - occasionally - they'll also need different props and data in order to achieve this.

Introducing RenderingTarget type

The RenderingTarget type will aim to encapsulate the different targets that DCR can render for. We'd likely start with just Web and Apps as options, but in the long term we could include AMP (Which DCR already renders for) and Editions which will come at a later date in the DCAR migration.

How can we define it?

There are really two options to define this new RenderingTarget type:

Option 1 - Enums

export enum RenderingTarget {
    Web,
    Apps,
    // Coming later?
    AMP,
    Editions,
}

Option 2 - String union

export type RenderingTarget = 
    | "Web"
    | "Apps"
    // Coming later?
    | "AMP"
    | "Editions"

While enums would likely make the most sense semantically, they're rarely used in DCR and generally we see string unions used in place.

This RenderingTarget type would then likely be heaving drilled in DCR (since we don't use React Context or any similar global state), to allow components to make decisions such as

<div>
      { renderingTarget === RenderingTarget.Apps ? <AppsLightbox  ... /> : <WebLightbox ... /> } 
</div>

It's worth noting that as the RenderingTarget type expands, these comparisons would become more difficult, leaving us a few options for handling differences between the rendering targets

<div>
     {{
           RenderingTarget.Web: <WebLightbox ... />
           RenderingTarget.Apps: <AppsLightbox  ... />
           RenderingTarget.AMP: <></>
           RenderingTarget.Editions: <></>
     }[renderingTarget]}
</div>

// OR

const renderLightbox= (renderingTarget) = {
     switch(renderingTarget) {
           case RenderingTarget.Web:
               return <WebLightbox ... />
           case RenderingTarget.Apps:
               return <AppsLightbox ... />
           default:
               return <></>
     }
} 

// ...
<div>
     {renderLightbox(renderingTarget)}
</div>

We should carefully consider how we want to define RenderingTarget now, and what kinds of solutions we may use in components in the future.

Differing props by the RenderingTarget type

While working on the original spike, we discovered that we may encounter differences between the required props for each rendering target. As an example, while in DCR we require a contributionsServiceUrl string in order to link users to sign up for a digital subscription, this wouldn't be required in the app - where it would likely be activated natively, through bridget.

As a consequence of this, we took a look at how we can differ types based on the value of the RenderingTarget.

Introducing the CombinedProps pattern

During the spike, @JamieB-gu came up with the follow pattern to enable specific Apps only & Web only props:

Definition:

type AppsProps<Props> = Props extends void
	? { renderingTarget: RenderingTarget.Apps }
	: Props & { renderingTarget: RenderingTarget.Apps };
type WebProps<Props> = Props extends void
	? { renderingTarget: RenderingTarget.Web }
	: Props & { renderingTarget: RenderingTarget.Web };

type AppsOrWeb<Apps, Web> = AppsProps<Apps> | WebProps<Web>;

export type CombinedProps<Common, Apps, Web> = Common & AppsOrWeb<Apps, Web>;

Usage:

/ * Component Defintion */
type CommonProps = {
	format: ArticleFormat;
	...
};

type AppsProps = void;

type WebProps = {
	contributionsServiceUrl: string;
};

const MyComponent = (
             // Because there are different definitions of `props` based the value of renderingTarget, we cannot deconstruct props in the function definition
	props: CombinedProps<CommonProps, AppsProps, WebProps>,
) => {
     const { 
        format, 
        renderingTarget,
        ... 
     } = props

    /* If we tried to access props.contributionServiceUrl here, we'd get a TS error, as it's only
        defined when we've checked that renderingTarget === RenderingTarget.Web */

     return (
          <>
               ...
               {renderingTarget === RenderingTarget.Web && (
                    <AnotherComponent contributionsServiceUrl={props.contributionsServiceUrl} />
               )}
          </>
     )
}

/* Calling the component */

// The component must be defined twice, once with the web props & once with the apps props:
<div>
{renderingTarget === RenderingTarget.Web ? 
       <MyComponent renderingTarget={RenderingTarget.Web} format={format} contributionServiceUrl={'...'} />
       :  <MyComponent renderingTarget={RenderingTarget.Apps} format={format} /> }
</div>

What are the advantages of this approach?

The key advantage of this approach is type safety - Not only do we know for sure which props are defined & when, but we also add safety that people won't try and develop apps features with web props, or vice versa.

Furthermore, it may help with the longevity of the code, as mixed rendering target props could result in eventual confusion on which props should be used when, making it more difficult for future developers to determine what should be used when.

What are the disadvantages of this approach?

This pattern is fairly complex, adding a new pattern to DCR with type definitions that may take some learning for people not familiar with typescript.

It also does not work if you deconstruct the props in the function definition, meaning we'd have a mix of prop deconstruction techniques in DCR going forward.

Another consideration is that this likely wouldn't be a massively common pattern in DCR, as not a lot of components would require differing props, meaning it could be even more jarring to come across.

Alternatives to the CombinedProps pattern

Make everything optional

In this case, we could just make contributionServiceUrl optional, and then do a check such as renderingTarget === RenderingTarget.Web && contributionServiceUrl !== undefined && <AnotherComponent contributionsServiceUrl={props.contributionsServiceUrl} />

For this to work best, you'd want to make DCR variants of the model optional, but have the FE model where DCR still expects contributionsServiceUrl to be defined for web requests. Otherwise we'd compromise the type safety of our model.

Have all the data, all the time

The second option would be to make all apps only and web only props required all the time. This relies on the idea that we will have a single data source for both apps & web, and we'll always be able to provide that data no matter what request we're making.

Both of the alternatives have the disadvantage that we lose the ability to discern the difference between apps and webs data at the point of usage, which could make the system feel more complex for developers.

---

Thanks for reading this long explanation of the two concepts we could bring to DCR! I'd love to hear your thoughts and ideas on how to solve this problem!

The key thing here is that adding the ability to render for apps requires adding some amount of new complexity to DCR, and we need to carefully think about how we manage this complexity, and what the best way to implement it could be!

[sndrs]

For this to work then, our components in DCR will need to be aware of what platform they're rendering for, and - occasionally - they'll also need different props and data in order to achieve this.

I would urge you really consider this in depth.

When AMP was added to frontend, the web templates were reused and peppered with conditional logic, and that made working on them extremely complicated.

It became very hard to find the bit you wanted to change, and to know if you were breaking a platform you were not intending to affect – this was the experience that drove the separation of AMP and web in DCR.

I think you would want to really understand exactly how contingent any current similarity across platforms is:

• if the current similarity suggests a DRYer solution, what is behind that similarity?
• could that ever change? have the platforms been more different before? why?
• how far do the similarities need to diverge before it's easier to have different components? what is it like to work the code at that point?

The more child components your component composes the more it matters (e.g. a whole page vs a timestamp span).

I totally understand why you'd want to reduce the surface area for edits when you want to push simultaneous changes out across multiple platforms, and why that's hard currently.

But binding distinct platforms in code implies a large amount of identity – does that genuinely obtain here or are the platforms just coincidentally similar right now?

It's useful to consider how easy any solution will make undoing it/deleting things (if you can't delete code then you're stuck with it).

Does forking by platform inside components (of all sizes) make this a lot harder?

[OllysCoding]

Thanks for your response @sndrs this historical context is super useful!

I think fundamentally we should see Apps & Web articles as the same, part of the product decision that went into the organisation of this work is that by moving to one platform, we're committing to this similarity.

However, we accept some levels of differences as necessary due to the difference in how each should run:

• Web/Browser, which DCR currently supports makes heavy use of full web technologies, needs inclusions of things like headers and footers, and uses our web implementations of CMP & reader revenue callouts.
• Webview in Apps, which we'll be adding support for in DCR will perform communication with the native app for a lot of what DCR uses web versions of, such as CMP reader revenue callouts and video players. We don't need the header and footer as these are provided natively.

Our approach for this during the spike we carried out took a balanced approach of duplication & in-component comparisons; we duplicated things like SSR code & article layouts, where significant differences in composition were present.

However - as an example - were we to want to have separate Lightbox implementations, and didn't want to go down the route of a Platform type prop, we'd need to create apps & web versions of the entire component tree - over 10 different components - where most if not all of the remaining code would be identical minus the inclusion of the platform specific component.

I think I see the inclusion of a platform type prop to be more similar to that of ArticleFormat: The structure mostly remains the same (minus layouts), but we make rendering decisions in some places based on the value.

While the separation of AMP code and Web code was somewhat successful in reducing complexity there are some factors to consider in this route

• AMP has remained largely unchanged, not a product priority which is given much attention, having code which is mostly separate is almost an advantage in this case
• In places where AMP imports web components, we've been almost unknowingly changing the AMP platform when working on these components, without much active thought or testing.

In the other hand, I'd argue that multi-platform complexity is desired where differences are required for the Apps/Webs work. Any engineer working on DCR should really be thinking about all platforms when working on their code, and most work that does not interact with a platform specific feature would be written & tested across both platforms.

tl;dr: We don't want people to be writing code for the 'apps' or 'webs' but rather just writing rendering code, and complexity which comes from having multiple platforms would (hopefully) be able to be managed in isolated components.

---

In terms of exit strategy, I think early on if we get the vibe that the complexity being added here is too much then we will have ample opportunity to change strategy during this migration - as long as we stay aware of the complexity we're adding :)

---

Also as a final note, if it helps the understanding, I wonder if we should switch the naming of the suggested Platform type (which is overloaded) to RenderingTarget (e.g RenderingTarget.apps, RenderingTarget.web) as I feel this might better reflect our goals?

[bryophyta]

Thanks for such a clear and detailed discussion Olly!

I don't think I currently know enough about the similarities/differences/future of web vs app rendering to comment on that part of the discussion above. But on the issue of the CombinedProps pattern, my 2 cents is that I think it's worth going for unless we can find an alternative that will also give us the same kind of type safety:

• Creating this sort of type system can be relatively complicated, but I'm not sure that using it is equally complicated.
• We can put thought into documenting the pattern up front -- e.g. at the very least I assume there will be a single source of truth for the RenderingTargets type, which will provide a central location for jsdoc documentation.
• Ultimately the business logic of determining which props are required for which rendering target is quite complicated. If we make everything required or optional we lose a source of documentation about this logic, as well as the type checking itself.

Having said all that, I think the above probably only applies if we can limit the surface area of the API. If the cases become more complex e.g. sub-types of specific rendering targets) and/or if we find it leaking into our other patterns (e.g. adding new props to patterns like decidePalette) then I'd think we should tread pretty carefully. Although I suspect this comes down more to Alex's points above, rather than to the type abstraction itself.

[ioannakok]

This is an excellent and very clear RFC - especially if we consider what problem you're trying to solve here, so well done and thanks! Same as Pete, I can't say I know enough about the similarities / differences between web and apps and how this might evolve in the future. However, my understanding is at this point we're talking about:

1. Very specific differences, such as headers, footers, CMP & reader revenue callouts,
2. Differences that may arise from the fact that DCR will now have to talk to the native app, e.g. videos

For these two cases CombinedProps seems a good solution to me because of the type safety it provides plus it makes the code very intentional which I believe is easier to understand than in the case of either making everything optional or passing all the props. Basically +1 on @bryophyta's point ". If we make everything required or optional we lose a source of documentation about this logic, as well as the type checking itself"!

From what I understand there is a third case which we can see arising in the future, i.e. if we want to have separate implementations of a feature / component. In your Lightbox example I believe CombinedProps sounds reasonable because of the complexity of the component. I am just not sure if in other cases this choice is going to be that straightforward. For example, in a lot less complex situation it might actually make sense to just have all props optional. I don't think we can answer this at the moment though (go for consistency or simplicity), just writing down my thoughts!

[OllysCoding]

Hi everyone!

Thanks for your excellent feedback on this RFC :) Having taken some to think through this feedback, here is the state of play:

---

I believe we should go ahead with the RFC as defined above, using the RenderingTarget enum type and the CombinedProps pattern, which will enable to us to start building layouts which can render in the apps.

During this process we will keep track of both the complexity we are adding to codebase, as well whether the patterns feel maintainable.

Furthermore, we should track the overall objectives of how we'd like developers to be able to work on DCR, looking at the cultural element of how we can help people approach work on the codebase.

Overall our goal should be to enable developers to work quickly and easily across any product (e.g articles, fronts) and for any rendering target (e.g apps, web).

In these early stages, backing out or making changes to either separation patterns, or the file structure (#7174) should be relatively easy, and we should feel confident to change any of these decisions at a later date.

---

I'll now be raising a PR to add in these pattern definitions, so that development work can continue on creating articles which can be rendered on apps.

[bryophyta]

Thanks for the update @OllysCoding -- that sounds like a good plan!

[jamesgorrie]

@OllysCoding your last comment would make a splendid ADR!

[mxdvl]

We created an ADR for using the context API in #8704

[mxdvl]

I would recommend going for a string union over an enum, as one is natively JavaScript and the other requires transformation by the TypeScript compiler.

// If you define a array `as const`…
const renderingTarget = [
    "Web",
    "Apps",
    "Editions",
] as const;

// … you can infer the type from it…
export type RenderingTarget = renderingTarget[number];

// … and create a custom type guard, if you so desire!
export isRenderingTarget = (target: string): target is RenderingTarget =>
	renderingTarget.map(String).includes(target)

[mxdvl]

We’ve settled on a simple union type: "Web" | "Apps"