Skip to content
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

Merged
merged 25 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8342952
trial using context for renderingTarget
cemms1 Aug 30, 2023
a2a5b54
refactor: one context to rule them all
cemms1 Aug 31, 2023
cac99e7
Merge branch 'main' into cemms1/use-context-for-rendering-target
cemms1 Aug 31, 2023
91cfa08
ensure rendering target is reflected in layout stories context
cemms1 Sep 1, 2023
1f24871
Merge branch 'main' into cemms1/use-context-for-rendering-target
cemms1 Sep 4, 2023
fbe778c
docs: add new react context proposal adr and rename adr sub dirs
cemms1 Sep 4, 2023
8414c5f
docs: add storybook docs to help explain context and/or decorators
cemms1 Sep 4, 2023
d92d8da
add context hook to improve ease of use of context object
cemms1 Sep 4, 2023
c3bfe7a
Merge branch 'main' into cemms1/use-context-for-rendering-target
cemms1 Sep 5, 2023
174def7
ensure useContext hook cannot be used outside of relevant provider
cemms1 Sep 6, 2023
9d41ac0
move provider down tree and prevent direct context use
cemms1 Sep 6, 2023
6ac0184
remove console.log
cemms1 Sep 6, 2023
095689e
rename context to config, simplify implementation, add islands support
cemms1 Sep 6, 2023
4bf6dec
test: add test for config context and mock for other tests
cemms1 Sep 6, 2023
9d34771
ignore linting issue for context with comment
cemms1 Sep 6, 2023
9162e02
copy lint config from AR to detect react version
cemms1 Sep 6, 2023
7006f21
Merge branch 'main' into cemms1/use-context-for-rendering-target
cemms1 Sep 6, 2023
7ae529d
docs: update react context ADR to remove ban on use within islands, s…
cemms1 Sep 6, 2023
55a5b21
improve docs, and readability by renaming config type
cemms1 Sep 6, 2023
40af5aa
move provider higher up the tree and include other pages
cemms1 Sep 7, 2023
06b26df
test: fix tests
cemms1 Sep 7, 2023
52f5edf
Merge branch 'main' into cemms1/use-context-for-rendering-target
cemms1 Sep 7, 2023
2a0d92a
docs: update jsdoc descriptions for page components
cemms1 Sep 7, 2023
5cd1902
docs: clarify context docs and update status, moving existing doc to …
cemms1 Sep 7, 2023
0b278e5
docs: spelling
cemms1 Sep 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions dotcom-rendering/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ module.exports = {
ignoreNonDOM: true,
},
],
// We want to be careful with context and certainly avoid unnecessary re-renders
'react/jsx-no-constructed-context-values': 'error',

'@typescript-eslint/switch-exhaustiveness-check': 'error',
'array-callback-return': 'error',
Expand Down Expand Up @@ -171,6 +173,10 @@ module.exports = {
},
settings: {
'import/resolver': 'typescript',
react: {
// Tells eslint-plugin-react to automatically detect the version of React to use
version: 'detect',
},
},
overrides: [
{
Expand Down
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>
);
};
2 changes: 2 additions & 0 deletions dotcom-rendering/.storybook/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Lazy } from '../src/components/Lazy';
import { Picture } from '../src/components/Picture';
import { mockRESTCalls } from '../src/lib/mockRESTCalls';
import { setABTests } from '../src/lib/useAB';
import { ConfigContextDecorator } from './decorators/configContextDecorator';

// Prevent components being lazy rendered when we're taking Chromatic snapshots
Lazy.disabled = isChromatic();
Expand Down Expand Up @@ -139,6 +140,7 @@ const guardianViewports = {
/** @type {import('@storybook/react').Preview} */
export default {
decorators: [
ConfigContextDecorator,
(Story) => {
storage.local.clear();
return Story();
Expand Down
24 changes: 24 additions & 0 deletions dotcom-rendering/docs/architecture/028-react-context-api.md
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
Expand Up @@ -15,4 +15,4 @@ the react context api to extract the `edition` property to prevent this.

## Status

Approved
Superseded by 028-react-context-api.md
10 changes: 10 additions & 0 deletions dotcom-rendering/docs/development/storybook.md
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.
5 changes: 5 additions & 0 deletions dotcom-rendering/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,11 @@ declare namespace JSX {
clientOnly?: boolean;
props: any;
children: React.ReactNode;
/**
* This should be a stringified JSON of `ConfigContext`
* @see /dotcom-rendering/src/types/configContext.ts
*/
config: string;
};
}

Expand Down
1 change: 1 addition & 0 deletions dotcom-rendering/scripts/gen-stories/get-stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export const ${storyVariableName} = () => {
);
};
${storyVariableName}.storyName = '${renderingTarget}: Display: ${displayName}, Design: ${designName}, Theme: ${theme}';
${storyVariableName}.args = { config: { renderingTarget: '${renderingTarget}' } };
`;
};

Expand Down
9 changes: 9 additions & 0 deletions dotcom-rendering/scripts/jest/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,12 @@ global.TextDecoder = TextDecoder as unknown as typeof global.TextDecoder;

// Mocks the version number used by CDK, we don't want our tests to fail every time we update our cdk dependency.
jest.mock('@guardian/cdk/lib/constants/tracking-tag');

// Mocks the useConfig hook in ConfigContext, so that we don't have to use the provider all the time
jest.mock('../../src/components/ConfigContext.tsx', () => {
const mockConfig = { renderingTarget: 'Web' };
return {
...jest.requireActual('../../src/components/ConfigContext.tsx'),
useConfig: () => mockConfig,
};
});
6 changes: 4 additions & 2 deletions dotcom-rendering/src/client/discussion.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { doHydration } from './islands/doHydration';
import { getEmotionCache } from './islands/emotion';
import { getConfig } from './islands/getConfig';
import { getProps } from './islands/getProps';

const forceHydration = async (): Promise<void> => {
Expand All @@ -12,14 +13,15 @@ const forceHydration = async (): Promise<void> => {
);
if (!guElement) return;

// Read the props from where they have been serialised in the dom using an Island
// Read the props and config from where they have been serialised in the dom using an Island
const props = getProps(guElement);
const config = getConfig(guElement);

// Now that we have the props as an object, tell Discussion we want it to expand itself
props.expanded = true;

// Force hydration
await doHydration(name, props, guElement, getEmotionCache());
await doHydration(name, props, guElement, getEmotionCache(), config);
} catch (err) {
// Do nothing
}
Expand Down
20 changes: 14 additions & 6 deletions dotcom-rendering/src/client/islands/doHydration.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 { Config } from '../../types/configContext';

declare global {
interface DOMStringMap {
Expand All @@ -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
Copy link
Contributor Author

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

*/
export const doHydration = async (
name: string,
data: { [key: string]: unknown } | null,
element: HTMLElement,
emotionCache: EmotionCache,
config: Config,
): Promise<void> => {
// If this function has already been run for an element then don't try to
// run it a second time
Expand All @@ -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>,
);
}

Expand Down
17 changes: 12 additions & 5 deletions dotcom-rendering/src/client/islands/doStorybookHydration.js
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';

Expand All @@ -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')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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$/ */
Expand All @@ -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
Expand All @@ -44,5 +51,5 @@ export const doStorybookHydration = () => {
),
);
}
});
}
};
28 changes: 28 additions & 0 deletions dotcom-rendering/src/client/islands/getConfig.ts
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;
}
};
38 changes: 28 additions & 10 deletions dotcom-rendering/src/client/islands/initHydration.ts
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';
Expand Down Expand Up @@ -36,14 +37,15 @@ export const initHydration = async (
): Promise<void> => {
const name = getName(element);
const props = getProps(element);
const config = getConfig(element);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 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?


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;
}
Expand All @@ -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
Expand All @@ -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);
}
}
};
Loading
Loading