-
Notifications
You must be signed in to change notification settings - Fork 29
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 24 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,24 @@ | ||
# 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 be thought of as a type of configuration for our application. | ||
- 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`) | ||
- Context should **only be used for React components** (ie. not for JSON responses or non-JSX helper code) | ||
- This is because the React context API will not work outside of React | ||
|
||
## Status | ||
|
||
Approved |
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 |
---|---|---|
@@ -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 { Config } 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): Config => { | ||
const serialised = marker.getAttribute('config'); | ||
|
||
try { | ||
if (!serialised) { | ||
throw Error('Unable to fetch config attribute from marker element'); | ||
} else { | ||
return JSON.parse(serialised) as Config; | ||
} | ||
} 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.
We are using the app config in a similar way to
emotionCache