Using context for renderingTarget

[cemms1]

There was an RFC put together earlier this year about how to go about implementing renderingTarget in DCR for apps work:
#7256

In this issue, it mentions the following:

This RenderingTarget type would then likely be heaving drilled in DCR (since we don't use React Context or any similar global state)

The fact that we don't use React Context or global state in DCR is not strictly true as it's used here:

dotcom-rendering/dotcom-rendering/src/components/ContentABTest.amp.tsx

Lines 47 to 94 in fd2beca

	const Context = React.createContext<ContentABTestContext | undefined>(
	undefined,
	);
	export const ContentABTestProvider = ({
	switches,
	pageId,
	children,
	}: {
	switches: Switches;
	pageId: string;
	children: React.ReactNode;
	}) => {
	const group = getGroup(pageId);
	const providerValue = switches.ampContentAbTesting ? { group } : {};
	return (
	<Context.Provider value={providerValue}>{children}</Context.Provider>
	);
	};
	/**
	* Obtain the current test group
	*
	* @example
	* const { group } = useContentABTestGroup();
	*
	* switch(group) {
	* case (0): {
	* // ...
	* }
	* case (1): {
	* // ...
	* }
	* // Handle other groups
	* }
	*
	*/
	export const useContentABTestGroup = () => {
	const context = React.useContext(Context);
	if (context === undefined) {
	throw Error(
	'useContentABTestGroup must be used within the ContentABTestProvider',
	);
	}
	return context;
	};

There are a number of ADRs from 2019 discussing React Context and why it was decided to largely avoid using it:

• https://github.com/guardian/dotcom-rendering/blob/fd2becac42c28cdaf9b6ee112d5c67eb23c535c5/dotcom-rendering/docs/architecture/016-react-context-api.md
• https://github.com/guardian/dotcom-rendering/blob/fd2becac42c28cdaf9b6ee112d5c67eb23c535c5/dotcom-rendering/docs/principles/developer-experience.md#explicit-props
• https://github.com/guardian/dotcom-rendering/blob/fd2becac42c28cdaf9b6ee112d5c67eb23c535c5/dotcom-rendering/docs/architecture/020-react-portals.md#react-context

The main ADR says

This decision should be revisited as the codebase grows.

Given that the codebase has grown considerably since 2019 and is still growing, it would be worth revisiting whether React Context could be useful in certain situations.

Specifically, it is worth considering using React Context for the case of renderingTarget (currently either "Web" or "Apps") as it is set at the entrypoint only and will not change throughout the component lifecycle.

Having started working on implementing renderingTarget for Bridget work as a team, PRs are becoming exhaustingly long due to unnecessary changes relating to prop drilling, both in terms of writing code and reviewing it.
This has prompted this discussion and a revisit to the prior decision of not using contexts in DCR.

[OllysCoding]

Specifically, it is worth considering using React Context for the case of renderingTarget (currently either "Web" or "Apps") as it is set at the entrypoint only and will not change throughout the component lifecycle.

This could be a good line in the sand for introducing context, I think that a lot of the concerns around context was that unmanaged use of global state would make the code hard to reason about especially if we ever needed to migrate away from DCR.

However, using context solely to hold static values specific to an individual render makes sense to me.

Something that will need to be figured out though is how to support islands when using react context - I'm guessing each island will need a context provider, which also means the islands hydration code will also need to know what was in context during the server-side-render, definitely would love to see a proof-of-concept around how that might work!

[cemms1]

Thanks for this Olly, that's super useful context (heh heh) to have here

[OllysCoding]

On the subject of lines in the sand, it could be worth adding that the main article/front/etc json body shouldn't ever end up in context - the risk being that these objects are often very large, and we don't want to be unnecessarily burdening the client with all that data when most of it will likely not be needed.

[abeddow91]

I think this is a great opportunity to test out context in the DCR codebase. As Olly mentioned, we should make sure we are strict / have lines in the sand about when we use context vs prop drilling to avoid mystifying the code base but a static value like the renderingTarget makes total sense.

[jamesgorrie]

There's some extra food for thought from @bryophyta.

And the original PR that sparked this ADR.

Paraphrasing @nicl's original comment

1. It's implicit, and thus hurts readability
2. It introduces global state
3. It was coupling us to react at a time the didn't know they wanted to use it
4. It breaks component re-usability/composability due to global state

Looking at each point individually:

1. Readability

I am unsure this is the case, especially given that prop-drilling introduces it's own readability and ability to decern where a property is actually introduces vs used. I think a PR as suggested would really help decide this

2. Global state

On @OllysCoding's point, it would be good to decide what is allowed into this global state e.g. it is static, it is truly global etc. We could reference this line in the side next the the generating code to help reviewers in the future consider their choice.

3. React

I would say we're pretty coupled. Not sure what others think?

4. Composability

This is an interesting point, but Context in the way that we are talking about it being used, to avoid prop drilling, often lends itself to be used at the leaf component node, avoiding coupling the whole app to global state but rather the root to the leaf component.

This also assumes that we have a shallow component tree, which is mentioned above, which isn't true given the size of the PRs that have instigated this.

e.g. Component1, Component2, Component3 are now coupled to prop1, but only Component4

flowchart LR
    Root-->|drills prop1| Component1-->|drills prop1| Component2-->|drills prop1| Component3-->|drills prop1| Component4["Component using prop 1"]

But now only Component4 is coupled to prop1 allowing better composability up the chain.

flowchart LR
    Root-->|"setContext(prop1)"| Component1--> Component2--> Component3--> Component4["Component useProp1()"]

---

As mentioned a lot above, I think defining the line in the sand will probably be the fun / tricky bit.

[arelra]

As others have said if we do use context we need some strict lines (context allowlist?) in the sand.

I wonder if one of the rules should be that we don't use context on the client. Working out why re-renders happen can be tricky at the best of times. If we are using context on the client and getting re-renders from context updates that will be harder still.

I do worry however that over time in order to avoid prop drilling, context is used as an escape hatch and we will start to put more and more props into context. Prop drilling can be tedious but it is very explicit and makes reasoning about what props a component required much simpler.

[jamesgorrie]

I really like the ssr-only rule. I know this came up with the discussion on WithABProvider.

The point on avoiding prop drilling made me think that it could promote people over abstracting components, creating a deeper component tree, where it might be best that that component is actually just 1 component.

But this would be is we are suggesting being able to use Context anywhere. My understanding of this discussion is, "Can we add a global Context to DCR for truly global values" vs "Should we just be using Context to avoid prop drilling".

@cemms1 ☝️ are we suggesting adding a Context with global values, or something more far reaching in Context's use?

[cemms1]

This is an interesting point that I hadn't considered in this discussion so far actually. We could introduce one global context to store global values (like renderingTarget) and use these values anywhere we need in the component tree.

I was initially thinking of using context because the amount of prop drilling for Bridget PRs using renderingTarget felt unsustainable and the vast majority of changes in the diff weren't anything to do with the "real" change.

We absolutely don't want to litter the codebase with loads of different contexts to maintain as this is a bit of a slippery slope. Using one global context might reduce temptation to unnecessarily use contexts and could be easier to "police" or monitor...

[cemms1]

Here's a suggestion of a way forward, using React Context for very generic rendering state where:

• it's specified at a high level
• the logic is contained in one place
• the context is static (state does not change between re-renders)

It does assume that most of the time, we'll be dealing with target: "Web" which is true at the moment but will possibly change throughout the DCR for apps migration.

It would be worth drafting a doc on what our lines in the sand are and then linking this in the context docstring to avoid overzealous use of context where it may not be the best choice for a solution.

Based on the comments on this thread already, here's an attempt:

Lines in the sand

Context should

• be considered global for rendered components in dotcom-rendering
• not be used for values which change between re-renders
• not be used for JSON responses or non JSX components (?)
  - only be used for server-side rendered components (should not be inside islands)

Any help on the documentation of this would be much appreciated!

[cemms1]

After a good chat with @arelra and @JamieB-gu, the idea of disallowing use of context within islands is not practical so this has been removed from our "lines in the sand".

We are also changing the name of this context to "config" to better reflect what its purpose is in our application.

There's an updated comment on the suggested PR here: #8704

[cemms1]

Thanks everyone for participating in this discussion!

The PR suggesting implementation for this (#8704) has been merged, with an additional issue here (#8795) to improve the efficiency of the islands code (not completed at the time of writing).

I'm going to close this discussion since we have reached a resolution, which is now documented in the code base ✨