Move theming APIs to core

[Andarist]

• adjust docs
• decide if we want to export ThemeContext from @emotion/core
• decide if we want to keep or remove withTheme
• changeset
• prepare a deprecation entry point for emotion-theming

cc @mitchellhamilton @JakeGinnivan @joltmode

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 6076eab

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[JakeGinnivan]

What is the go with the remove theming RFC? If the plan is to migrate theming to hooks then maybe that should be the play instead?

[Andarist]

What is the go with the remove theming RFC? If the plan is to migrate theming to hooks then maybe that should be the play instead?

We believe in everything that is written in the RFC. The bare context seems to make more sense because it solves multiple problems - and the only downside is that you have to import and call the hook to get access to the context. We have decided to keep the theme in the core, at least for the time being, because v11 focus is to migrate internals to hooks and doing some minor cleanups. People would most likely get upset about removing the built-in theme. Ideally, I'd like to incorporate those docs from the RFC into published docs - just the tone will be a little bit different (after all we are not removing theme right now). Out of curiosity - what do you think about arguments presented there and how would you feel about built in theme removal?

[JakeGinnivan]

Has withTheme been removed from emotion itself? If not it these types should probably stay in

[Andarist]

I've removed it as part of this PR - but the decision about it is not final. It seems like in hooks world this is not needed. I know class components can't use hooks but they can do smth like that:

// this example is not generic, but also `withTheme` tends to be used sparingly, so this might not be a problem
const WithTheme = props => {
  const theme = useTheme()
  return <SomeClass theme={theme} {...props}/>
}

So I don't know if we should keep builtin withTheme.

[JakeGinnivan]

We have pretty much migrated to hooks, but it has taken time. I think at a minimum you should add in the upgrade notes a snippet to polyfill it.

[Andarist]

Sure, a migration path for each change is going to be documented. I think this one even could be automated with codemods, just need to consider people wrapping components with multiple HOCs at once.

[JakeGinnivan]

I have no issues with theme being removed actually which is why I asked. I thought the RFC was pretty reasonable. Then again the planned simplification of theme also is nice.

Just thinking that if the plan is to remove theming then it probably doesn't make sense to make these breaking changes now to just have users break again with v12 or whatever it is when theming is removed entirely.

I do like the approach of documenting for libraries to to theming using hooks as documented in that RFC. It removes the conflicts issue and keeps theme in core for normal usage?

[emmatown]

I'm thinking what we might want to do is export exactly:

export let ThemeContext = React.createContext({})
export let useTheme = () => useContext(ThemeContext)

I think this would be really good because it would have parity with the "create your own theme" approach so rather than thinking about theming as there being two approaches, there would only be the one approach but we would have a "default" one that you can use.

[JakeGinnivan]

At that point you can't just destructure theme from props like you can do now, and you would still need to introduce the global theme type etc for these types.

Unsure including those 2 lines would provide much value over documenting them in the docs.

[emmatown]

I still mean that we would pass the theme to styled interpolations and the css prop. I more mean we shouldn't have withTheme and a ThemeProvider component that merges the theme with the parent in the component.

[tomsseisums]

While this wouldn't affect me, but @JakeGinnivan has a good point in here when thinking about the wider ecosystem:

Just thinking that if the plan is to remove theming then it probably doesn't make sense to make these breaking changes now to just have users break again with v12 or whatever it is when theming is removed entirely.

[JakeGinnivan]

I also use the merging capability of ThemeProvider a fair bit, so if we just exported the context then you would have to do the merging yourself.

We could also keep the ThemeProvider but it has an optional prop for the context to use?

Though this is seeming like maybe we should just keep what we have, simplify as much as we can and then docs for how libraries should do theming to not conflict with the main app

[Andarist]

Just thinking that if the plan is to remove theming then it probably doesn't make sense to make these breaking changes now to just have users break again with v12 or whatever it is when theming is removed entirely.

I don't mind keeping theme builtin indefinitely, but mostly because I think this is what people want and that removing it would lead to an unnecessary negative backlash. Major versions are here to introduce some breaking changes though, so I don't think that breaking people "again" is necessarily that bad. Moving this at a slower pace means more gradual migration than all at once - which might be in turn better. cc @mitchellhamilton

I do like the approach of documenting for libraries to to theming using hooks as documented in that RFC. It removes the conflicts issue and keeps theme in core for normal usage?

That's the current status of this, especially given that we don't know if we ultimately want to remove the builtin theme or not.

I more mean we shouldn't have withTheme and a ThemeProvider component that merges the theme with the parent in the component.

I agree that withTheme seems unnecessary (OTOH it shouldn't cost us that much to keep it 🤷‍♂ ), but I'm not sure about ThemeProvider. Even though this abstraction is thin it is really convenient - I also don't believe this is some new API that has to be learned much, because it works really similarly to setState. The API doesn't do much, but what it does seems valuable to me.

We could also keep the ThemeProvider but it has an optional prop for the context to use?

I kinda like that 👍

[emmatown]

I don't mind keeping theme builtin indefinitely

Agreed.

---

@JakeGinnivan and others, would you be able to share real world usages of ThemeProvider - especially usages that aren't at the top level?

[Andarist]

For example smth like which parses current theme and provides inverted colors for its subtree.

[JakeGinnivan]

I agree that withTheme seems unnecessary (OTOH it shouldn't cost us that much to keep it 🤷‍♂ ), but I'm not sure about ThemeProvider. Even though this abstraction is thin it is really convenient - I also don't believe this is some new API that has to be learned much, because it works really similarly to setState. The API doesn't do much, but what it does seems valuable to me.

Agree, I think my comment was directed at removing the typescript but not the actual implementation. I think we should remove both.

[JakeGinnivan]

As for real world, I don't have anything which changes colours based on other colours. But we build and augment the theme at different levels

Here are some examples:

export const thewest: Theme = {
    kind: Product.TheWest,
    fonts: fonts.thewest,
    margins: metrics.thewest.margins,
    cards: metrics.thewest.cards,
    componentMetrics: metrics.thewest.componentMetrics,

    // This following properties are provided through route resolution in provide-route-resolved-data-to-theme.tsx
    fallbackImages: undefined as any,
    section: 'default' as 'default',
    sectionValues: sections.thewest.default,
}

<ThemeProvider theme={thewest}>
  <App />
</ThemeProvider>

Then inside the routing we have:

 <ThemeProvider
            theme={{
                fallbackImages,
                section,
                sectionValues,
            }}
        >
            {children}
        </ThemeProvider>

This is how we do
https://7news.com.au/
vs
https://7news.com.au/sunrise

export const OverrideThemeSection: React.FC<Props> = props => {
    const { children, section } = props
    return <ThemeProvider theme={{ section }}>{children}</ThemeProvider>
}

And this allows us to theme a particular part of a page in a theme from another section of the site.

[emmatown]

Okay, let's keep ThemeProvider and we might as well keep withTheme as well since it can be tree-shaken

[codecov]

Codecov Report

Merging #1628 into next will not change coverage.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/core/src/context.js	100% <ø> (ø)	⬆️
packages/core/src/jsx.js	100% <ø> (ø)	⬆️
packages/core/src/class-names.js	100% <ø> (ø)	⬆️
packages/core/src/global.js	97.56% <ø> (ø)	⬆️
packages/primitives-core/src/styled.js	100% <100%> (ø)	⬆️
packages/styled/src/base.js	100% <100%> (ø)	⬆️
packages/core/src/theming.js	100% <100%> (ø)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6076eab:

Sandbox	Source
Emotion	Configuration

[Andarist]

This is pretty ready to be merged in. A final review would be appreciated.

Other than that there is only a single thing I'd like to decide on - should we export "raw" ThemeContext from the @emotion/core package?

[emmatown]

Other than that there is only a single thing I'd like to decide on - should we export "raw" ThemeContext from the @emotion/core package?

Yes because people might want to use ThemeContext.Consumer or ThemeContext.Provider directly.

[JakeGinnivan]

Yes because people might want to use ThemeContext.Consumer or ThemeContext.Provider directly.

Why would you use ThemeContext over ThemeProvider out of interest, that would mean there is two ways to do the same thing, one just has more features?

[JakeGinnivan]

Yes because people might want to use ThemeContext.Consumer or ThemeContext.Provider directly.

Why would you use ThemeContext over ThemeProvider out of interest, that would mean there is two ways to do the same thing, one just has more features?

To clarify, over ThemeProvider and useTheme.

[emmatown]

For ThemeProvider, not wanting the merging behavior.

For useTheme, using ThemeContext.Consumer and static contextType = ThemeContext in a class component