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

RFC: use React Context to pass server side tests #7126

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { ArticleDesign, ArticleDisplay, ArticlePillar } from '@guardian/libs';
import { ArticleContainer } from './ArticleContainer';
import { ArticleHeadline } from './ArticleHeadline';
import { Flex } from './Flex';
import { LeftColumn } from './LeftColumn';
import { Section } from './Section';
import {
mockServerSideTestsProviderFactory,
useServerSideTests,
} from './ServerSideTestProvider';

export default {
component: ArticleHeadline,
title: 'Components/ArticleHeadlineServerTestDemo',
};

const format = {
display: ArticleDisplay.Standard,
design: ArticleDesign.Standard,
theme: ArticlePillar.News,
};

const ArticleHeadlineWithTest = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm creating this component in the story file just to demonstrate what a component with a test would look like. We can imagine that this is a regular component in the components/ directory which has had test code added to it.

const { activeServerSideTests } = useServerSideTests();
return (
<>
<ArticleHeadline
headlineString="This is how the default headline looks"
format={format}
tags={[]}
webPublicationDateDeprecated=""
/>
{activeServerSideTests.myTest == 'variant'
? '~*^_ the variant version _^*~'
: ''}
</>
);
};

export const VariantStory = () => {
return (
<Section fullWidth={true}>
<Flex>
<LeftColumn borderType="full">
<></>
</LeftColumn>
<ArticleContainer format={format}>
<ArticleHeadlineWithTest />
</ArticleContainer>
</Flex>
</Section>
);
};
VariantStory.story = {
name: 'Variant Demo',
decorators: [mockServerSideTestsProviderFactory({ myTest: 'variant' })],
};

export const ControlStory = () => {
return (
<Section fullWidth={true}>
<Flex>
<LeftColumn borderType="full">
<></>
</LeftColumn>
<ArticleContainer format={format}>
<ArticleHeadlineWithTest />
</ArticleContainer>
</Flex>
</Section>
);
};
ControlStory.story = {
name: 'Control Demo',
decorators: [mockServerSideTestsProviderFactory({ myTest: 'control' })],
};
116 changes: 63 additions & 53 deletions dotcom-rendering/src/web/components/ArticlePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { FocusStyles } from './FocusStyles.importable';
import { Island } from './Island';
import { Metrics } from './Metrics.importable';
import { ReaderRevenueDev } from './ReaderRevenueDev.importable';
import { ServerSideTestProvider } from './ServerSideTestProvider';
import { SetABTests } from './SetABTests.importable';
import { SkipTo } from './SkipTo';

Expand All @@ -34,61 +35,70 @@ type Props = {
export const ArticlePage = ({ CAPIArticle, NAV, format }: Props) => {
return (
<StrictMode>
<Global
styles={css`
/* Crude but effective mechanism. Specific components may need to improve on this behaviour. */
/* The not(.src...) selector is to work with Source's FocusStyleManager. */
*:focus {
${focusHalo}
}
::selection {
background: ${brandAlt[400]};
color: ${neutral[7]};
}
`}
/>
<SkipTo id="maincontent" label="Skip to main content" />
<SkipTo id="navigation" label="Skip to navigation" />
{(format.design === ArticleDesign.LiveBlog ||
format.design === ArticleDesign.DeadBlog) && (
<SkipTo id={'key-events-carousel'} label="Skip to key events" />
)}
<Island clientOnly={true} deferUntil="idle">
<AlreadyVisited />
</Island>
<Island clientOnly={true} deferUntil="idle">
<FocusStyles />
</Island>
<Island clientOnly={true} deferUntil="idle">
<Metrics
commercialMetricsEnabled={
!!CAPIArticle.config.switches.commercialMetrics
}
<ServerSideTestProvider tests={CAPIArticle.config.abTests}>
Copy link
Contributor Author

@bryophyta bryophyta Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only significant change in this file should be adding this ServerSideTestProvider wrapper (the rest is just indentation)

<Global
styles={css`
/* Crude but effective mechanism. Specific components may need to improve on this behaviour. */
/* The not(.src...) selector is to work with Source's FocusStyleManager. */
*:focus {
${focusHalo}
}
::selection {
background: ${brandAlt[400]};
color: ${neutral[7]};
}
`}
/>
</Island>
<Island clientOnly={true} deferUntil="idle">
<BrazeMessaging idApiUrl={CAPIArticle.config.idApiUrl} />
</Island>
<Island clientOnly={true} deferUntil="idle">
<ReaderRevenueDev
shouldHideReaderRevenue={
CAPIArticle.shouldHideReaderRevenue
}
<SkipTo id="maincontent" label="Skip to main content" />
<SkipTo id="navigation" label="Skip to navigation" />
{(format.design === ArticleDesign.LiveBlog ||
format.design === ArticleDesign.DeadBlog) && (
<SkipTo
id={'key-events-carousel'}
label="Skip to key events"
/>
)}
<Island clientOnly={true} deferUntil="idle">
<AlreadyVisited />
</Island>
<Island clientOnly={true} deferUntil="idle">
<FocusStyles />
</Island>
<Island clientOnly={true} deferUntil="idle">
<Metrics
commercialMetricsEnabled={
!!CAPIArticle.config.switches.commercialMetrics
}
/>
</Island>
<Island clientOnly={true} deferUntil="idle">
<BrazeMessaging idApiUrl={CAPIArticle.config.idApiUrl} />
</Island>
<Island clientOnly={true} deferUntil="idle">
<ReaderRevenueDev
shouldHideReaderRevenue={
CAPIArticle.shouldHideReaderRevenue
}
/>
</Island>
<Island clientOnly={true} deferUntil="idle">
<FetchCommentCounts repeat={true} />
</Island>
<Island clientOnly={true}>
<SetABTests
abTestSwitches={filterABTestSwitches(
CAPIArticle.config.switches,
)}
pageIsSensitive={CAPIArticle.config.isSensitive}
isDev={!!CAPIArticle.config.isDev}
/>
</Island>
<DecideLayout
CAPIArticle={CAPIArticle}
NAV={NAV}
format={format}
/>
</Island>
<Island clientOnly={true} deferUntil="idle">
<FetchCommentCounts repeat={true} />
</Island>
<Island clientOnly={true}>
<SetABTests
abTestSwitches={filterABTestSwitches(
CAPIArticle.config.switches,
)}
pageIsSensitive={CAPIArticle.config.isSensitive}
isDev={!!CAPIArticle.config.isDev}
/>
</Island>
<DecideLayout CAPIArticle={CAPIArticle} NAV={NAV} format={format} />
</ServerSideTestProvider>
</StrictMode>
);
};
76 changes: 76 additions & 0 deletions dotcom-rendering/src/web/components/ServerSideTestProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import type { PartialStoryFn } from '@storybook/addons/dist/ts3.9/types';
import type { ComponentProps, JSXElementConstructor } from 'react';
import React from 'react';
import type { ServerSideTests } from '../../types/config';

const ServerSideTestContext = React.createContext<
{ activeServerSideTests: ServerSideTests } | undefined
>(undefined);

export const ServerSideTestProvider = ({
tests,
children,
}: {
tests: ServerSideTests;
children: React.ReactNode;
}) => (
<ServerSideTestContext.Provider value={{ activeServerSideTests: tests }}>
{children}
</ServerSideTestContext.Provider>
);

/**
* Get current server side tests
*
* @example
* const { activeServerSideTests } = useServerSideTests();
*
* if (activeServerSideTests.myTestName == "variant") {
* //...
* } else {
* //...
* }
*
*/
export const useServerSideTests = () => {
const context = React.useContext(ServerSideTestContext);

if (context === undefined) {
throw Error(
'useServerSideTests must be used within the ContentABTestProvider',
);
}

return context;
};

/**
Copy link
Contributor Author

@bryophyta bryophyta Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've adapted the wrapper component and the hook code above from ContentABTest.tsx in AMP (which in turn follows the pattern recommended by Kent C. Dodds.

The Storybook decorator is something new that I've come up with today (which probably shows when you read the code!)

It feels like Storybook/Chromatic might be one of the main pain points for using Context in this way, so I thought it would be worth trying to create a user-friendly way to mock the ServerSideTestsProvider in Storybook.

Using a factory here doesn't feel ideal tbh. I ended up doing it to make the type declarations easier (for me, at least!) but I'm sure that it could be rewritten in a more user-friendly way if the pattern is adopted.

*
* @param tests
* @returns A Storybook decorator function which will wrap a story in a <ServerSideTestProvider>
* containing the data passed to the factory in the `tests` parameter.
* This decorator should receive a type parameter corresponding to the top-level
* component in the story.
*
* @example
export default {
component: MyComponent,
title: 'Components/MyComponent',
decorators: [
mockServerSideTestsProviderFactory({myTest: "control"})<typeof MyComponent>,
],
};
*/
export function mockServerSideTestsProviderFactory(tests: ServerSideTests) {
return function <
/* eslint-disable @typescript-eslint/no-explicit-any -- Can we narrow this? */
C extends keyof JSX.IntrinsicElements | JSXElementConstructor<any>,
/* eslint-enable @typescript-eslint/no-explicit-any */
>(Story: PartialStoryFn<ComponentProps<C>>) {
return (
<ServerSideTestProvider tests={tests}>
<Story />
</ServerSideTestProvider>
);
};
}