SWC-5612 - Store application-wide data (including auth token) in context #1047
Conversation
nickgros
changed the title
| jest.mock('../../../lib/containers/widgets/SynapsePlot', () => { | ||
| return { | ||
| __esModule: true, | ||
| default: function MockSynapsePlot() { | ||
| return <div role="figure"></div> | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| jest.mock('../../../lib/containers/widgets/SynapseImage', () => { | ||
| return { | ||
| __esModule: true, | ||
| default: function MockSynapseImage() { | ||
| return <div role="img"></div> | ||
| }, | ||
| } | ||
| }) |
There was a problem hiding this comment.
Simple example for how we can use mocks to reduce testing surface while using RTL's render or Enzyme's mount
| import React from 'react' | ||
| import { | ||
| SynapseContextProvider, | ||
| SynapseContextType, | ||
| } from '../lib/utils/SynapseContext' | ||
|
|
||
| export const MOCK_ACCESS_TOKEN = 'mock-access-token' | ||
|
|
||
| export const MOCK_CONTEXT_VALUE: SynapseContextType = { | ||
| accessToken: MOCK_ACCESS_TOKEN, | ||
| utcTime: false, | ||
| isInExperimentalMode: false, | ||
| } | ||
|
|
||
| export const MOCK_CONTEXT = React.createContext(MOCK_CONTEXT_VALUE) | ||
|
|
||
| /** | ||
| * Full context object with default values for testing. | ||
| */ | ||
| export const SynapseTestContext = jest | ||
| .fn() | ||
| .mockImplementation(({ children }) => { | ||
| return ( | ||
| <SynapseContextProvider synapseContext={MOCK_CONTEXT_VALUE}> | ||
| {children} | ||
| </SynapseContextProvider> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Handy prebuilt pieces to use across all tests.
| * | ||
| * Adapted from https://medium.com/@jrwebdev/react-higher-order-component-patterns-in-typescript-42278f7590fb | ||
| */ | ||
| export const withQueryClientProvider = <P extends object>( |
There was a problem hiding this comment.
No longer needed as we now require all components to be wrapped in SynapseContextProvider, which itself contains a QueryClientProvider
| * @param param0 | ||
| * @returns | ||
| */ | ||
| export const SynapseContextProvider: React.FunctionComponent<SynapseContextProviderProps> = ({ |
There was a problem hiding this comment.
Optional reading: here's a blog post by Kent C. Dodds that has guided many of the design decisions made here.
| /** | ||
| * TODO: SWC-5612 - Replace token prop with SynapseContext.accessToken | ||
| * | ||
| * This wasn't done because Enzyme's shallow renderer is not currently | ||
| * compatible with the `contextType` field in the React 16+ context API. | ||
| * | ||
| * This can be fixed by rewriting tests to not rely on the shallow renderer. | ||
| * | ||
| * See here: https://github.com/enzymejs/enzyme/issues/1553 | ||
| */ | ||
|
|
There was a problem hiding this comment.
e.g. comment I left on the few classes I didn't want to rewrite tests for
| export default function useGetEntityBundle( | ||
| accessToken: string, | ||
| entityId: string, | ||
| bundleRequest: EntityBundleRequest = ALL_FIELDS, | ||
| version?: number, | ||
| options?: UseQueryOptions<EntityBundle, SynapseClientError, EntityBundle>, | ||
| ) { | ||
| const { accessToken } = useSynapseContext() | ||
| return useQuery<EntityBundle, SynapseClientError>( | ||
| ['entitybundle', entityId, version, bundleRequest], | ||
| [accessToken, 'entitybundle', entityId, version, bundleRequest], | ||
| () => | ||
| SynapseClient.getEntityBundleV2( | ||
| entityId, |
There was a problem hiding this comment.
I think this is awesome: this query function is a hook itself, so we can call useContext here. The caller of useGetEntityBundle doesn't even need the access token!
| const defaultQueryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| staleTime: 30 * 1000, // 30s | ||
| retry: false, // SynapseClient knows which queries to retry | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Moved this from QueryClientWrapper, we'll provide it here.
| const context = useContext(SynapseContext) | ||
| if (context === undefined) { | ||
| throw new Error( | ||
| 'useSynapseContext must be used within a SynapseContextProvider', |
| * | ||
| * This can be fixed by rewriting tests to not rely on the shallow renderer. | ||
| * | ||
| * See here: https://github.com/enzymejs/enzyme/issues/1553 |
|
Fully reviewed SynapseContext.tsx, useEntityBundle.ts, HasAccess.tsx, FacetNavPanel.tsx. |
Summary of changes
These changes let us pull the authentication token (as well as some other fields) from context, instead of passing them through props. This dramatically cleans up the code by eliminating some excessive prop drilling. This also reduces the risk of error: it was easy to not pass a token via props since it was optional in most contexts. Now the token, is only accessed when it is used.
This will be a breaking change. Any application that uses SRC will need to wrap all SRC components in a
SynapseContextProvider.The application data provided via this context currently includes these values:
The context also provides a
react-queryQueryClient, so our components can now utilize a shared cache when usingreact-query.This can be extended to provide more data fairly easily, if the need arises.
A note on testing
I discovered that Enzyme doesn't fully support the React 16+ context API (see Enzyme React 16 support and enzymejs/enzyme#2189). In particular,
contextType(which is used exclusively on class components) doesn't work with the shallow renderer. I rewrote tests for a few components to usemount, but for other class components, I left the access token as a prop. An access token can still be passed to the component via context using aSynapseContextConsumer. Additionally, components accepting access tokens as a prop must still be wrapped in context in live settings, because they may have child components that access context.Tangentially, Facebook no longer uses Enzyme in any new tests. I think we should consider at least avoiding the shallow renderer (we can still use Jest mocks to reduce the test surface of complex components).
Checklist
For follow up: