-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
trial using context for renderingTarget #8704
Changes from 18 commits
8342952
a2a5b54
cac99e7
91cfa08
1f24871
fbe778c
8414c5f
d92d8da
c3bfe7a
174def7
9d41ac0
6ac0184
095689e
4bf6dec
9d34771
9162e02
7006f21
7ae529d
55a5b21
40af5aa
06b26df
52f5edf
2a0d92a
5cd1902
0b278e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { ConfigProvider } from '../../src/components/ConfigContext'; | ||
|
||
const defaultConfig = { renderingTarget: 'Web' }; | ||
|
||
export const ConfigContextDecorator = (Story, { args: { config } }) => { | ||
const context = { ...defaultConfig, ...config }; | ||
|
||
// For easy debugging | ||
console.log('Storybook application config: \n', context); | ||
|
||
return ( | ||
<ConfigProvider value={context}> | ||
<Story /> | ||
</ConfigProvider> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# React Context API | ||
|
||
## Context | ||
|
||
[A decision](dotcom-rendering/docs/architecture/016-react-context-api.md) was made in 2019 to avoid use of the React context API, preferring prop-drilling and avoiding global state. | ||
|
||
[This decision was revisited in 2023](https://github.com/guardian/dotcom-rendering/discussions/8696) due to [the introduction of rendering target as a prop](dotcom-rendering/docs/architecture/proposed-adrs/rendering-type.md) which represents a global state, resulting in very heavy prop-drilling throughout the app. This began to complicate pull requests due to so many unrelated changes appearing in the diff since the props needed to be provided at every layer of the app, as well as tightly coupling components unnecessarily. | ||
|
||
An RFC was put together [here](https://github.com/guardian/dotcom-rendering/pull/8704) to trial using the React context API for this specific type of global state, which doesn't change throughout the component lifecycle ie. immutable application configuration. | ||
|
||
## Decision | ||
|
||
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) | ||
- Context should **not be used for values that change between re-renders**, since this adds unnecessary complexity and there are alternative solutions for these cases. There is a eslint rule to attempt to prevent this (`react/jsx-no-constructed-context-values`) | ||
|
||
## Status | ||
|
||
Proposed | ||
cemms1 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Storybook | ||
|
||
We use Storybook to visualise our components in an isolated environment, where we can tweak the conditions as we want. | ||
We use Chromatic for visual regression testing of Storybook components. | ||
|
||
## Rendering context | ||
|
||
We use context for static, global state in the dotcom-rendering app, so every story is wrapped in a context provider component. In the real world, our top component includes this context provider. | ||
|
||
In Storybook is largely invisible as it's hidden within the [configuration](dotcom-rendering/.storybook). There's a decorator configured to wrap around stories and log the context output to the console, for easier debugging. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ import { CacheProvider } from '@emotion/react'; | |
import { log, startPerformanceMeasure } from '@guardian/libs'; | ||
import { createElement } from 'react'; | ||
import { createRoot, hydrateRoot } from 'react-dom/client'; | ||
import { ConfigProvider } from '../../components/ConfigContext'; | ||
import type { ApplicationConfig } from '../../types/configContext'; | ||
|
||
declare global { | ||
interface DOMStringMap { | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using the app config in a similar way to |
||
*/ | ||
export const doHydration = async ( | ||
name: string, | ||
data: { [key: string]: unknown } | null, | ||
element: HTMLElement, | ||
emotionCache: EmotionCache, | ||
config: ApplicationConfig, | ||
): Promise<void> => { | ||
// If this function has already been run for an element then don't try to | ||
// run it a second time | ||
|
@@ -58,16 +62,20 @@ export const doHydration = async ( | |
element.querySelector('[data-name="placeholder"]')?.remove(); | ||
const root = createRoot(element); | ||
root.render( | ||
<CacheProvider value={emotionCache}> | ||
{createElement(module[name], data)} | ||
</CacheProvider>, | ||
<ConfigProvider value={config}> | ||
<CacheProvider value={emotionCache}> | ||
{createElement(module[name], data)} | ||
</CacheProvider> | ||
</ConfigProvider>, | ||
); | ||
} else { | ||
hydrateRoot( | ||
element, | ||
<CacheProvider value={emotionCache}> | ||
{createElement(module[name], data)} | ||
</CacheProvider>, | ||
<ConfigProvider value={config}> | ||
<CacheProvider value={emotionCache}> | ||
{createElement(module[name], data)} | ||
</CacheProvider> | ||
</ConfigProvider>, | ||
); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { createElement } from 'react'; | ||
import { createRoot } from 'react-dom/client'; | ||
import { ConfigProvider } from '../../components/ConfigContext'; | ||
import { getConfig } from './getConfig'; | ||
import { getName } from './getName'; | ||
import { getProps } from './getProps'; | ||
|
||
|
@@ -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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is eslint automatically fixing on save |
||
if (element instanceof HTMLElement) { | ||
const name = getName(element); | ||
const props = getProps(element); | ||
const config = getConfig(element); | ||
|
||
if (!name) return; | ||
if (element.getAttribute('deferuntil') === 'hash') return; | ||
if (!name) continue; | ||
if (element.getAttribute('deferuntil') === 'hash') continue; | ||
|
||
import( | ||
/* webpackInclude: /\.importable\.tsx$/ */ | ||
|
@@ -34,7 +37,11 @@ export const doStorybookHydration = () => { | |
.querySelector('[data-name="placeholder"]') | ||
?.remove(); | ||
const root = createRoot(element); | ||
root.render(createElement(module[name], props)); | ||
root.render( | ||
<ConfigProvider value={config}> | ||
{createElement(module[name], props)} | ||
</ConfigProvider>, | ||
); | ||
}) | ||
.catch((e) => | ||
// eslint-disable-next-line no-console -- We want to log here | ||
|
@@ -44,5 +51,5 @@ export const doStorybookHydration = () => { | |
), | ||
); | ||
} | ||
}); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import type { ApplicationConfig } from '../../types/configContext'; | ||
|
||
/** | ||
* getConfig takes the given html element and returns its config attribute | ||
* | ||
* We expect the element to always be a `gu-*` custom element | ||
* | ||
* @param marker : The html element that we want to read the config attribute from; | ||
* @returns | ||
*/ | ||
export const getConfig = (marker: HTMLElement): ApplicationConfig => { | ||
const serialised = marker.getAttribute('config'); | ||
|
||
try { | ||
if (!serialised) { | ||
throw Error('Unable to fetch config attribute from marker element'); | ||
} else { | ||
return JSON.parse(serialised) as ApplicationConfig; | ||
} | ||
} catch (error: unknown) { | ||
console.error( | ||
`🚨 Error parsing config. Is this data serialisable? ${String( | ||
serialised, | ||
)} 🚨`, | ||
); | ||
throw error; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import type { EmotionCache } from '@emotion/cache'; | ||
import { doHydration } from './doHydration'; | ||
import { getConfig } from './getConfig'; | ||
import { getName } from './getName'; | ||
import { getProps } from './getProps'; | ||
import { onInteraction } from './onInteraction'; | ||
|
@@ -36,14 +37,15 @@ export const initHydration = async ( | |
): Promise<void> => { | ||
const name = getName(element); | ||
const props = getProps(element); | ||
const config = getConfig(element); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
if (!name) return; | ||
|
||
const deferUntil = element.getAttribute('deferuntil'); | ||
switch (deferUntil) { | ||
case 'idle': { | ||
whenIdle(() => { | ||
void doHydration(name, props, element, emotionCache); | ||
void doHydration(name, props, element, emotionCache, config); | ||
}); | ||
return; | ||
} | ||
|
@@ -52,25 +54,35 @@ export const initHydration = async ( | |
whenVisible( | ||
element, | ||
() => { | ||
void doHydration(name, props, element, emotionCache); | ||
void doHydration( | ||
name, | ||
props, | ||
element, | ||
emotionCache, | ||
config, | ||
); | ||
}, | ||
{ rootMargin }, | ||
); | ||
return; | ||
} | ||
case 'interaction': { | ||
onInteraction(element, (targetElement) => { | ||
void doHydration(name, props, element, emotionCache).then( | ||
() => { | ||
targetElement.dispatchEvent(new MouseEvent('click')); | ||
}, | ||
); | ||
void doHydration( | ||
name, | ||
props, | ||
element, | ||
emotionCache, | ||
config, | ||
).then(() => { | ||
targetElement.dispatchEvent(new MouseEvent('click')); | ||
}); | ||
}); | ||
return; | ||
} | ||
case 'hash': { | ||
if (window.location.hash.includes(name) || hasLightboxHash(name)) { | ||
void doHydration(name, props, element, emotionCache); | ||
void doHydration(name, props, element, emotionCache, config); | ||
} else { | ||
// If we didn't find a matching hash on page load, set a | ||
// listener so that we check again each time the reader | ||
|
@@ -80,14 +92,20 @@ export const initHydration = async ( | |
window.location.hash.includes(name) || | ||
hasLightboxHash(name) | ||
) { | ||
void doHydration(name, props, element, emotionCache); | ||
void doHydration( | ||
name, | ||
props, | ||
element, | ||
emotionCache, | ||
config, | ||
); | ||
} | ||
}); | ||
} | ||
return; | ||
} | ||
default: { | ||
return doHydration(name, props, element, emotionCache); | ||
return doHydration(name, props, element, emotionCache, config); | ||
} | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
We can't use the React context API for non-React components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense! thank you!