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

Use Context to serve the edition prop #801

Closed
wants to merge 2 commits into from

Conversation

oliverlloyd
Copy link
Contributor

What does this change?

Adds an EditionContext provider that removes the need to keep passing edition around through multiple levels and replaces it with the Provider / Consumer model so it can be accessed as and when required.

Why?

In a typical React application, data is passed top-down (parent to child) via props, but this can be cumbersome for certain types of props (e.g. locale preference, UI theme) that are required by many components within an application. Context provides a way to share values like these between components without having to explicitly pass a prop through every level of the tree.
https://reactjs.org/docs/context.html

Note on Hooks

React Hooks were added in 16.8 and include the useContext hook which means the syntax for using context is now very clean and readable. There's no longer any need to wrap components in <Providers> and instead you can just use const edition = useContext(EditionContext); ❤️

How to use

If your component wants to access the edition then you need 2 lines of code:

import { EditionContext } from '@frontend/web/context/EditionContext';

and then, in your function:

const edition = useContext(EditionContext);

Hooks allow classes to be replaced by functional components so ☝️ should always be sufficient but if you didn't want to migrate to hooks and needed to access edition from a class you can use the, still perfectly good, render props pattern:

<EditionContext.Consumer>
  {edition => /* render something based on the edition value */}
</EditionContext.Consumer>

How to test context

If your test depends on the edition value then you can provide it to your test html stack in the same way as it's given to the actual app:

        const { container } = render(
            <EditionContext.Provider value={'AUS'}>
                {/* and now the useContext call inside ReaderRevenueLinks will return AUS */}
                <ReaderRevenueLinks
                    urls={urls}
                    dataLinkNamePrefix={'nav2 : '}
                    noResponsive={false}
                />
            </EditionContext.Provider>,
        );

Link to supporting Trello card

https://trello.com/c/2C74DiyP/744-spike-react-context

@PRBuilds
Copy link

PRBuilds commented Oct 4, 2019

PRbuilds results:

💚 AMP validation
amp-report.txt

LightHouse Reporting
1570181189.report.html

--automated message

@gtrufitt
Copy link
Contributor

gtrufitt commented Oct 4, 2019

Have added a few interested parties 👋

@gtrufitt
Copy link
Contributor

gtrufitt commented Oct 4, 2019

I'm keen on this but had a question in Chat:

A question might be how does this fit with the DS team (I'll ping Simon on the PR) and functional components where everything required to render is passed to them. Does relying on context in a component mean all projects require context? Or can you split out that model and in some cases pass edition and others context.

@JamieB-gu
Copy link
Contributor

👍 for a great description, very clear!

@oliverlloyd
Copy link
Contributor Author

Moving the my comments on chat to this PR:

The downsides to context are it means you have this airy, magical value that you have to know about. Prop drilling is explicit to it's clear where the data is. But 🤷 sometime prop drillings gets a bit nuts and can be a real pain and context fixes that - so long as you use it carefully, and don't go crazy and put everything in it!

The advantage is simplification, and this benefit only grows as we scale. Prop drilling really starts to become an evil, mind numbing nightmare once you begin breaking components out to handle additional complexity.

Copy link
Contributor

@nicl nicl left a comment

Choose a reason for hiding this comment

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

I am not a big fan of this for a couple of reasons:

  1. it is as people have mentioned implicit ('magical' as it's sometimes called) which can seriously harm readabiliity even if it helps concision
  2. it is introducing effectively global state
  3. it is coupling us more tightly to React at a time when we are not sure we even want to use it
  4. it breaks component re-usability/composability (related to the fact it is essentially a global)

But we can discuss IRL - happy to discuss, especially if most people think it is a good idea and disagree with the above!

@oliverlloyd
Copy link
Contributor Author

Closed for now. This is something we could revisit later if we feel the component tree has grown in complexity enough to justify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants