trial using context for renderingTarget #8704

cemms1 · 2023-08-30T17:00:58Z

What does this change?

Trials using React Context for renderingTarget instead of heavy prop drilling.

Why?

This PR suggests how we might use React context for the scenario of a property set on entrypoint and that doesn't change throughout the component lifecycle.

Resolves #8711
Off the back of this discussion #8696

Additional notes

I've left the renderingTarget prop in for the components from ArticlePage -> StandardLayout / LiveLayout (via DecideLayout).

This is because it became quite tricky to decouple the new props structure of WebProps | AppsProps, due to the renderingTarget being used as the identifier for this discriminated union.
It may not be necessary to decouple though, since this is only a few layers of props and since in this case the props have a purpose (not using them simply to pass down to lower layers of the component tree).

github-actions · 2023-08-30T17:15:41Z

Size Change: 0 B 🆕

Total Size: 0 B

_{compressed-size-action}

cemms1 · 2023-08-31T16:30:30Z

dotcom-rendering/src/components/ArticleMeta.stories.tsx

+export const ArticleAppsStory = () => {
+	return (
+		<Wrapper>
+			<ArticleMeta
+				format={{
+					display: ArticleDisplay.Standard,
+					design: ArticleDesign.Standard,
+					theme: Pillar.News,
+				}}
+				pageId=""
+				webTitle=""
+				byline="Lanre Bakare"
+				tags={tagsWithLargeBylineImage}
+				primaryDateline="Sun 12 Jan 2020 18.00 GMT"
+				secondaryDateline="Last modified on Sun 12 Jan 2020 21.00 GMT"
+				isCommentable={false}
+				discussionApiUrl=""
+				shortUrlId=""
+				ajaxUrl=""
+				showShareCount={true}
+			/>
+		</Wrapper>
+	);
+};
+ArticleAppsStory.args = { renderingContext: { target: 'Apps' } };
+


Adding this additional story to demonstrate how we can use the renderingContextDecorator to override the default rendering context (which has been set as "Web" in the decorator itself)

I like this addition to demonstrate this! Perhaps we could put a link to it in a more centralised place of documentation as a reference of "how to override the default rendering context". Not sure where the best place is though 🤔

cemms1 · 2023-08-31T16:32:48Z

dotcom-rendering/src/types/renderingContext.ts

+import type { RenderingTarget } from './renderingTarget';
+
+/**
+ * Context for global values (generic)
+ *
+ * This should not contain any properties which are likely to change between re-renders
+ */
+export interface RenderingContextType {
+	target: RenderingTarget;
+}


Any additional properties to the context must be added here.

We are still forming our lines in the sand but this could be a good place to add a linting rule to prevent unnecessary things from being added to the context if we want to do that?

Or potentially link out to a doc in the docstring?

From #8696 and our lines in the sand sounds like there is already consensus in:

no usage of context in Islands

not be used for JSON responses or non JSX components

I am wondering whether we could create our own custom eslint rules for those two. Sounds like these are good cases for an automated check.

Custom ESLint rules can be done without too much hassle: https://stackoverflow.com/a/40979625/1577447

cemms1 · 2023-08-31T16:35:27Z

dotcom-rendering/.storybook/decorators/renderingContextDecorator.js

+import { RenderingContext } from '../../src/components/RenderingContext';
+
+const defaultContext = { target: 'Web' };
+
+export const RenderingContextDecorator = (
+	Story,
+	{ args: { renderingContext } },
+) => {
+	const context = { ...defaultContext, ...renderingContext };
+
+	// For easy debugging
+	console.log('Storybook rendering context: \n', context);
+
+	return (
+		<RenderingContext.Provider value={context}>
+			<Story />
+		</RenderingContext.Provider>
+	);
+};


This Storybook decorator wraps all stories in a <RenderingContext.Provider> component, with the default value of the context provided which can be overridden at component level

ioannakok · 2023-09-04T12:11:30Z

Thanks for raising the PR, putting thought on this taking the time to write the RFC! This makes sense to me for this occasion ("a global Context to DCR for truly global values" as per comment). Plus, it is always therapeutic to see repetitive code being removed. Just leaving some thoughts.

ioannakok · 2023-09-04T12:12:17Z

As per the doc for deprecated ADRs should the following be part of this PR?

Move this to historic ADRs and changing the status to Deprecated
Creating an updated ADR?

chrislomaxjones · 2023-09-04T12:11:46Z

dotcom-rendering/src/components/BylineLink.tsx

@@ -137,10 +132,12 @@ export const BylineLink = ({
 		);
 	});

+	const { target } = useContext(RenderingContext);


Hope you don't mind some non-webex input here, but super supportive of using contexts in the specific cases of passing around some immutable data!

What would you think of wrapping the call useContext(RenderingContext) up into its own function? I copied something similar into AMP from the now-defunct AB test context.

const { target } = useRenderingTarget();

One enhancement off the back of this is you could pre-compute isApps and isWeb and optionally destructure them when you use the hook e.g.

const useRenderingTarget = () => { // ... return { target, isApps: target === "Apps", isWeb: target === "Web", }; }; // usages const { isApps } = useRenderingTarget(); const { isWeb } = useRenderingTarget();

I like this idea!

Definitely keen for non WebX input here as this repo is shared by many, thanks v much Chris!

+1 on this suggested abstraction! I'm pretty sure I copied @chrislomaxjones's pattern from the AMP AB test code when I was putting together an RFC to use context for server-side tests.

Looking into that code also led me to How to use React Context effectively by Kent Dodds which I remember finding useful. Thought I'd link just in case it hasn't been surfaced in discussion already :)

fwiw, though I don't work with DCR anymore, rendering targets seem to me like a great use-case for Context and this looks like a great PR!

After trialing this solution, I do really like the derived values of context to help remove unnecessary logic from other places but think this would be best as a phase 2 type thing as this PR is a bit long already!

I've implemented what you've recommended @chrislomaxjones and @bryophyta in terms of the custom hook which is a better and safer use of context. The neat thing in that blog @bryophyta was avoiding specifying a default value and erroring with use outside of the provider. This has been super valuable input, thank you both!

Makes sense, liking the useConfig naming a lot.

…ince not practical

cemms1 · 2023-09-06T16:29:28Z

This PR has changed quite substantially since my conversation with @JamieB-gu and @arelra earlier today, who both had very good feedback on the approach taken and naming conventions, thank you both! 🙌 .

To keep it clear what we're dealing with here, we've agreed on calling the context "config", since that most succinctly describes the type of information we are allowing to exist in this context. Config must be static and immutable (and must not change between renders). It cannot be used outside of the <ConfigProvider /> and therefore can only be used in React components.

It is impossible to separate islands (client-side rendered components) from SSR (server-side rendered) components so we now add a <ConfigProvider /> to the island hydration code. We supply the config prop in a very similar way to props, by stringifying the object in the <gu-island> component and un-stringifying (?) it when hydrating.

cemms1 · 2023-09-06T16:41:54Z

dotcom-rendering/src/client/islands/doStorybookHydration.js

@@ -16,13 +18,14 @@ import { getProps } from './getProps';
 *    snapshots
 */
 export const doStorybookHydration = () => {
-	document.querySelectorAll('gu-island').forEach((element) => {
+	for (const element of document.querySelectorAll('gu-island')) {


this is eslint automatically fixing on save

cemms1 · 2023-09-06T16:43:44Z

dotcom-rendering/src/client/islands/doHydration.tsx

@@ -26,12 +28,14 @@ declare global {
 * @param data The deserialised props we want to use for hydration
 * @param element The location on the DOM where the component to hydrate exists
 * @param emotionCache An instance of an emotion cache to use for the island
+ * @param config Application configuration to be passed to the config context for the hydrated component


We are using the app config in a similar way to emotionCache

cemms1 · 2023-09-06T16:45:23Z

dotcom-rendering/src/components/ArticlePage.tsx

-				<>
-					<LightboxLayout
-						imageCount={article.imagesForLightbox.length}
+			<ConfigProvider value={configContext}>


The ArticlePage components are wrapped in the ConfigProvider

cemms1 · 2023-09-06T17:02:29Z

dotcom-rendering/src/lib/ArticleRenderer.tsx

+	/**
+	 * Where is this coming from?
+	 * Config value is set at high in the component tree within a React context
+	 * @see /dotcom-rendering/src/components/ArticlePage.tsx or look for a `<ConfigProvider />`
+	 */
+	const { renderingTarget } = useConfig();
+


Added these comments as an attempt to make this pattern easier to understand on first visit

mxdvl · 2023-09-06T21:01:50Z

dotcom-rendering/src/components/Island.tsx

+			props={JSON.stringify(children.props)}
+			clientOnly={clientOnly}
+			rootMargin={rootMargin}
+			config={JSON.stringify(config)}


Is this the exact same data serialised for each and every island?

this config value comes from the useConfig hook, so it's the same for every component in the same rendering request

We'll probably end up repeating the same data a lot as it's the same config for each island. I've added a comment to initHydration, but I wonder if we could put config in one place and have islands look it up from there, instead of passing it as an attribute to every island element?

The hydration code isn't JSX/React so I can't use a context hook there to fetch the config, which is why I thought of copying the same method that props uses.

The only way I can think to do this without repeating the config for each island is by putting the config on window.guardian, which is something I wanted to try to avoid if possible...

It might be that I'm trying to avoid it for no reason at all as this might be a better pattern to follow? Is there another way to do this without using window.guardian?

You could serialise with JSON.stringify inside a single script tag with type application/json on the page?

To @mxdvl's suggestion here's an example I came across recently of using the script tag in apps-rendering

Great suggestions, all. Thank you v much, this is very useful info.

As this PR has been around for a while now, I'm going to try to avoid this it ballooning in size so I'm going to treat this as an optimisation and have ticketed up here: #8795

georgeblahblah

This looks great! 🚀🚀🚀

I had a question/suggestion about copying config for each island, off the back of @mxdvl's question, but I don't think it's blocking.

dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md

georgeblahblah · 2023-09-07T14:55:01Z

dotcom-rendering/docs/architecture/proposed-adrs/react-context-api.md

+The decision to allow use of context more generally rests on the following _"lines in the sand"_:
+
+-   Context should be considered **global, static, and immutable** for rendered components in dotcom-rendering. It can act as a type of configuration for our application.
+-   Context should **only be used for JSX components** (ie. not for JSON responses or non-JSX helper code)


I'm think I'm missing something, but I wasn't sure what this means in practice. Do we have any counter-examples of when/why context isn't good for these use-cases?

Context should be considered global, static, and immutable for rendered components in dotcom-rendering. It can act as a type of configuration for our application.

The reason we don't want it to be mutable is to avoid unnecessary re-renders caused by context changing as this could cause lots of issues

Context should only be used for JSX components (ie. not for JSON responses or non-JSX helper code)

We can't use the React context API for non-React components

makes sense! thank you!

dotcom-rendering/src/components/ArticlePage.tsx

dotcom-rendering/src/components/FrontPage.tsx

georgeblahblah · 2023-09-07T15:12:51Z

dotcom-rendering/src/client/islands/initHydration.ts

@@ -36,14 +37,15 @@ export const initHydration = async (
 ): Promise<void> => {
 	const name = getName(element);
 	const props = getProps(element);
+	const config = getConfig(element);


related to @mxdvl's question about if config will be the same for every island (which I think it will be), I wonder if we need to getConfig from this element, or if we could store page config once for the page, and each island picks it up from there instead of every island having an identical config attribute?

georgeblahblah · 2023-09-07T15:13:49Z

dotcom-rendering/src/components/Island.tsx

+			props={JSON.stringify(children.props)}
+			clientOnly={clientOnly}
+			rootMargin={rootMargin}
+			config={JSON.stringify(config)}


We'll probably end up repeating the same data a lot as it's the same config for each island. I've added a comment to initHydration, but I wonder if we could put config in one place and have islands look it up from there, instead of passing it as an attribute to every island element?

…historic dir

ioannakok

Fantastic work!!! 🚀 Also agree with @mxdvl and @georgeblahblah's suggestions about islands but leaving this up to you. Well done!

arelra

Great work @cemms1 seeing this through!

Extracting out the serialisation of context outside of the component tree feels like a small optimisation that we can consider when/if we put a proper store in place that we can then facade with hooks.

cemms1 · 2023-09-08T10:38:18Z

Accepted strange Chromatic diff for Vimeo component after double checking that the component looks fine on Storybook and in the article https://www.theguardian.com/film/2023/jan/25/oscars-2023-how-to-watch-almost-every-nominated-feature-doco-and-short-in-australia when running the application locally. The visual diff appears to only happen on Chromatic so is being treated as a false positive for now

trial using context for renderingTarget

8342952

cemms1 added 🥒WiP Work in Progress DCR for Apps labels Aug 30, 2023

github-actions bot added the dotcom-rendering label Aug 30, 2023

refactor: one context to rule them all

a2a5b54

cemms1 force-pushed the cemms1/use-context-for-rendering-target branch from 3f494e9 to a2a5b54 Compare August 31, 2023 16:29

cemms1 commented Aug 31, 2023

View reviewed changes

Merge branch 'main' into cemms1/use-context-for-rendering-target

cac99e7

cemms1 added run_chromatic Runs chromatic when label is applied and removed 🥒WiP Work in Progress labels Aug 31, 2023

cemms1 marked this pull request as ready for review August 31, 2023 17:18

cemms1 requested a review from a team as a code owner August 31, 2023 17:18

ensure rendering target is reflected in layout stories context

91cfa08

chrislomaxjones reviewed Sep 4, 2023

View reviewed changes

cemms1 added RFC and removed run_chromatic Runs chromatic when label is applied labels Sep 4, 2023

cemms1 added 2 commits September 4, 2023 15:35

Merge branch 'main' into cemms1/use-context-for-rendering-target

1f24871

docs: add new react context proposal adr and rename adr sub dirs

fbe778c

cemms1 mentioned this pull request Sep 4, 2023

feat: implement filterSeenArticles in DCR #8695

Closed

cemms1 added 6 commits September 4, 2023 17:47

docs: add storybook docs to help explain context and/or decorators

8414c5f

add context hook to improve ease of use of context object

d92d8da

Merge branch 'main' into cemms1/use-context-for-rendering-target

c3bfe7a

ensure useContext hook cannot be used outside of relevant provider

174def7

move provider down tree and prevent direct context use

9d41ac0

remove console.log

6ac0184

cemms1 added 6 commits September 6, 2023 16:09

rename context to config, simplify implementation, add islands support

095689e

test: add test for config context and mock for other tests

4bf6dec

ignore linting issue for context with comment

9d34771

copy lint config from AR to detect react version

9162e02

Merge branch 'main' into cemms1/use-context-for-rendering-target

7006f21

docs: update react context ADR to remove ban on use within islands, s…

7ae529d

…ince not practical

cemms1 added the run_chromatic Runs chromatic when label is applied label Sep 6, 2023

improve docs, and readability by renaming config type

55a5b21

cemms1 commented Sep 6, 2023

View reviewed changes

mxdvl reviewed Sep 6, 2023

View reviewed changes

cemms1 added 2 commits September 7, 2023 11:52

move provider higher up the tree and include other pages

40af5aa

test: fix tests

06b26df

georgeblahblah approved these changes Sep 7, 2023

View reviewed changes

cemms1 added 3 commits September 7, 2023 16:17

Merge branch 'main' into cemms1/use-context-for-rendering-target

52f5edf

docs: update jsdoc descriptions for page components

2a0d92a

docs: clarify context docs and update status, moving existing doc to …

5cd1902

…historic dir

ioannakok approved these changes Sep 8, 2023

View reviewed changes

arelra approved these changes Sep 8, 2023

View reviewed changes

docs: spelling

0b278e5

cemms1 mentioned this pull request Sep 8, 2023

Improve efficiency of context in islands code #8795

Open

cemms1 merged commit ad53a6b into main Sep 8, 2023
10 checks passed

cemms1 deleted the cemms1/use-context-for-rendering-target branch September 8, 2023 10:38

mxdvl mentioned this pull request Sep 22, 2023

No mocked Config context provider #8896

Merged

trial using context for renderingTarget #8704

trial using context for renderingTarget #8704

Conversation

cemms1 commented Aug 30, 2023 • edited

What does this change?

Why?

Additional notes

github-actions bot commented Aug 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannakok Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemms1 Aug 31, 2023 • edited

Choose a reason for hiding this comment

ioannakok commented Sep 4, 2023 • edited

ioannakok commented Sep 4, 2023

chrislomaxjones Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryophyta Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemms1 commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxdvl Sep 7, 2023 • edited

Choose a reason for hiding this comment

ioannakok Sep 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeblahblah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannakok left a comment

Choose a reason for hiding this comment

arelra left a comment

Choose a reason for hiding this comment

cemms1 commented Sep 8, 2023

cemms1 commented Aug 30, 2023 •

edited

github-actions bot commented Aug 30, 2023 •

edited

ioannakok Sep 4, 2023 •

edited

cemms1 Aug 31, 2023 •

edited

ioannakok commented Sep 4, 2023 •

edited

chrislomaxjones Sep 4, 2023 •

edited

bryophyta Sep 4, 2023 •

edited

mxdvl Sep 7, 2023 •

edited

ioannakok Sep 8, 2023 •

edited