Should we require a ThemeProvider?

[emplums]

Right now in Primer React Components we have theme defaults set up in the event that consumers of the package don't have a ThemeProvider at the top of their component tree. We do this so that Primer Components is usable out of the box without any extra set up.

The way that we set a default theme is by setting it in defaultProps for every component:
https://github.com/primer/components/blob/master/src/Avatar.js#L30

If the theme is undefined, we use our own theme, if the user passes in a custom theme we use that theme.

We also set a default theme in our get function, technically right now I don't think this is necessary if we are making sure the theme is always defined via defaultProps:
https://github.com/primer/components/blob/master/src/constants.js#L9

If we remove the defaultProps theme, built in styles won't break because of the backup in the get function. But any styles created via system props that reference the theme will break (ex. color="blue.5")

The Problem

• It's easy to forget to add theme to defaultProps and if it's forgotten in a component file theming will be broken for that component (could be solved with a test)
• We have to manually pass the theme prop down to any nested components in a component tree, which we have forgotten in the past and created bugs. Requiring a ThemeProvider in some ways makes the component library more resilient in this regard - we'll never have to worry about whether or not the theme object is accessible if we assume that everyone is using a ThemeProvider
• Goes around the standard API for providing a theme. In other component libraries using styled-components you must use a ThemeProvider to give all components access to the theme
• Because users of the package don't need to use a ThemeProvider, using system props in the @primer/styled-octicons package won't work if there's no ThemeProvider. We don't set defaultProps in @primer/styled-octicons to avoid having a dependency on @primer/components.

Open Questions

Are these issues problematic enough to warrant asking everyone to use a ThemeProvider? After writing this out I'm not 100% convinced

• Will our work on color modes will change our perspective?

Urgency

This is fairly urgent to decide on because it's blocking getting the new Octicons out. Either outcome will effect how we decide to integrate the new @primer/styled-octicons package into Primer React Components

[BinaryMuse]

Some thoughts from Slack:

I guess thinking about it the other way — why have a default theme prop at all? It's so that components render properly without needing to set an explicit theme. I think the reason I hesitate is that hiding important details of the external API (specifying a theme) never feels quite right, but in our case since there's only one theme it's not that big a deal. If a user does want to use a specific theme, they either have to pass it to every component (to override our defaults) or use a ThemeProvider anyway, which arguably is the correct way to use the library. (could even be rolled into <BaseStyles /> or whatever)

On the other hand, the convenience isn't necessarily a terrible thing, and can be nice for one-off testing of components — at least now, with the single theme. I wonder if it will feel differently if and when we have 2/3

If we have multiple supported themes, specifying the theme you want explicitly forces you to actually think about which you want. Right now it's just an extra line of code you have to add with no real meaning or choice (unless you develop your own version of our theme, which is kinda hard due to the way the theme is currently structured)

Additionally, with multiple themes in place, our get function won't be able to safely fall back to a static theme value like we have now — arguably this should just throw an error rather than silently falling back.

[BinaryMuse]

@primer/styled-octicons demonstrates another problem with the current approach: it is difficult to build Primer-styled components outside of the Primer Components package and include them in the PC bundle due to cyclic dependencies; additionally, if users create their own components that want to use the same theme as the official PC components, they'll also need to set a default prop for the theme or else risk not getting appropriate styles if the user of the component chooses not to use the provider.

Moving this choice out of defaultProps into a provider call explicitly moves the responsibility out of every single component and to the end-user/application root for ensuring the theme applies to the entire React tree correctly, no matter where the component actually lives.

[BinaryMuse]

After talking with @emplums a bit, I'm starting to second-guess our decisions around the styled-octicons package (#821). Here are my thoughts:

• StyledOcticon is a Primer-specific thing, and it feels natural to have this inside Primer Components, along with all the other components that depend on Primer's theme and specific combination of system props.
• Given our current defaultProps theme situation, StyledOcticon has a dependency on Primer Components, but Primer Components also has a dependency on StyledOcticon because some components inside PC display Octicons.
• The prior API for StyledOcticon (<StyledOcticon {...systemProps} icon={Octicons.Icon} />) feels pretty natural to me and is what I'd expect from a library like Primer wrapping a separate icon library.
• The composition-based API (<StyledOcticon {...systemProps} icon={Octicons.Icon} />) is generic enough to work with any icon that renders an SVG (doesn't have to necessarily be Octicons). (We could change the name to just Icon and it might feel a little less awkward, e.g.: <Icon mr={2} icon={Octicons.Zap} />)

I still think that requiring an icon library as a peerDep feels a little weird, and I'm not sure that accidental icon drift is a good enough reason to make this a peerDep instead of a regular dep. That said, if we keep the current setup with the peerDep, I think the worst of the issues will go away if we stop using defaultProps.theme and instead require a theme be provided via a provider.

[colebemis]

As of #1094, consumers are required to wrap their application with the ThemeProvider component.