RFC: use React Context to pass server side tests #7126
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryophyta
commented
Feb 3, 2023
| theme: ArticlePillar.News, | ||
| }; | ||
|
|
||
| const ArticleHeadlineWithTest = () => { |
There was a problem hiding this comment.
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.
bryophyta
commented
Feb 3, 2023
| commercialMetricsEnabled={ | ||
| !!CAPIArticle.config.switches.commercialMetrics | ||
| } | ||
| <ServerSideTestProvider tests={CAPIArticle.config.abTests}> |
There was a problem hiding this comment.
the only significant change in this file should be adding this ServerSideTestProvider wrapper (the rest is just indentation)
bryophyta
commented
Feb 3, 2023
| return context; | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
|
Size Change: +193 B (0%) Total Size: 1.97 MB
ℹ️ View Unchanged
|
⚡️ Lighthouse report for the changes in this PRLighthouse tested 2 URLs Report for Article
Report for Front
|
bryophyta
changed the title
bryophyta
changed the title
|
"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days" |
|
This PR was closed because it has been stalled for 3 days with no activity. |
|
Closing this because we don't have capacity to discuss and implement at the moment. A note for future travellers, though: as far as I'm aware this pattern should work, so next time we're looking at prop-drilling server-side test info it might be worth checking this PR out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
useServerSideTestshook which wraps a React Context provider.Why?
Sometimes we need to access server-side test switches in DCR.
The options currently available:
<Island>then it can access thewindow.guardianconfig on the client side, which includes server-side test information.Our component tree is very deep in some places (e.g. elements of a fronts card could easily be 10 levels down from the root component which has direct access to the switch data). This makes prop drilling more time-consuming and error-prone. Generally we have an established preference for prop drilling in DCR but for temporary tests the cost of adding and then removing multiple levels of prop drilling seems disproportionately high.
Alternatives to the status quo:
The React Context approach avoids prop drilling and uses an idiomatic React pattern to do so.
Precedents
Pitfalls
'Magic' creep
As mentioned above, the accepted pattern in DCR is to favour prop drilling in part because it's more explicit, and relies less on the kind of 'magic' which can make a codebase hard to reason about.
In this RFC I am not suggesting that we should give up this pattern in general. Using the wrapper and custom hook allows us to limit the footprint of Context to a specific use-case, and also allows the Context Provider to be read-only. In this way I don't think it sets a precedent of reaching for Context as a catch-all shortcut.
Fragments and Storybook
If we put the
ServerSideTestProviderat the top level of each of the main rendering components (as I have inArticlePage.tsxin this branch) then most components should have the context if and when they need it. But this doesn't apply to Storybook stories, or any other place where we might render components outside these main routes.As things stand, the
useServerSideTestshook will throw a runtime error if it's used outside of theServerSideTestProvider. We could rewrite it to catch the error (or not throw it in the first place) but it's probably good to have it fail loudly.I've added a decorator function which could be used in Storybook to mock the context provider. Because it's a decorator, it can be applied to individual stories or to whole sets of stories for a component, which should reduce boilerplate.
That said, we might still face an inverse prop-drilling issue, whereby adding a context hook to any child component requires at least one of its parent components to contain the context provider. (e.g. adding a test to cards will break all the container stories.) That would be quite annoying. One way around this might be to create a global Storybook decorator which yields 'control' for any test name that's queried. Generally speaking we'll want our storybook to reflect the 'control' versions of components anyway, and there's nothing to stop us from opting in to the 'variant' for particular stories using something like the mocking function I've sketched here.
Nevertheless, the fact that tooling doesn't seem to be able to easily identify whether the relevant context will be present feels like a bit of a negative.
Tooling
Throwing a runtime error if the hook is used outside of the Provider scope should mean that issues are caught before they're deployed. But it would be nice if it could fail earlier (i.e. by provoking linter or compiler errors). I don't know if there's a good option for doing this.