Quick pass at removing theme generic param

[JakeGinnivan]

Having a go at just removing the Theme generic param

[Andarist]

I believe this should be Interpolation<Emotion.Theme>. With the proposed approach here, in this PR, you indeed don't need to parametrize GlobalProps with Theme but you still need to provide the default theme type~ to Interpolation here (and probably in some other types where you have removed this).

[JakeGinnivan]

Interpolation defaults to { theme: Emotion.Theme } so should be fine.

[Andarist]

Actually it doesn't from what I can see - https://github.com/emotion-js/emotion/blob/b81d652e70c8c421aae24e8fb357be6ff16143f0/packages/serialize/types/index.d.ts#L68

In here (for a Global case) it should be just Interpolation<Emotion.Theme> and not Interpolation<{ theme: Emotion.Theme }>

[JakeGinnivan]

Check out https://github.com/joltmode/emotion/pull/1/files#diff-e68603b09cd6bb7bbc9c8690f15e5392

I updated those types as part of this. But I didn't spend too much time on them, more putting this up as a general idea for joltmode if he wanted to go this way :)

[Andarist]

Gotcha - I think I was looking as original PR @joltmode looking for Interpolation changes 👍

The point about Global stands though.

[JakeGinnivan]

Dang, I get it. Good catch

[JakeGinnivan]

Fixed

[tomsseisums]

So with this change, it is intended that in order to use a theme, one will always have to provide the custom Emotion.Theme definition?

(Or ultimately the module based definition, as @mitchellhamilton suggested here: emotion-js#1609 (comment))

[JakeGinnivan]

So with this change, it is intended that in order to use a theme, one will always have to provide the custom Emotion.Theme definition?

Yep, the only downside is in say a mono repo where different applications have different themes.
But I’m not sure how well it will work supporting both models at the same time

[tomsseisums]

Yep, the only downside is in say a mono repo where different applications have different themes.
But I’m not sure how well it will work supporting both models at the same time

But doesn't that depend on how one configures the monorepo? It's still possible to expose different typings to different sub-projects.

Another topic is how well does the IDE of choice support monorepos and different TypeScript configs.

[JakeGinnivan]

I was thinking this, project references for instance.

You could basically declare the theme per project, so common code would extend all the themes? Because we can't have a union type I'm not sure how well that would work. Or common code extend a common interface, then leaf projects have a full one?

Something like that could work ok. This is untested territory for me though

[tomsseisums]

I guess, what remains until we can merge together, is to settle if this hardcoded global theme can actually operate in the real world. As it is quite an opinionated solution.

As I said, it's fine by me. I bet it's also fine with Jake, as he proposed the changes.

What about you @Andarist and the rest of emotion team?

[Andarist]

I guess, what remains until we can merge together, is to settle if this hardcoded global theme can actually operate in the real world. As it is quite an opinionated solution.

I believe this is just a matter of proper documentation. This IMHO should be used only if you are an application developer and you have a single theme type. If you want to have different themes (like in a monorepo where you have a single application but teams want to have their "own" them) and if you are a library (built with emotion) developer then you should use a custom theme.

Basically builtin theme has its shortcomings which can't be fixed and problems with it are outlined in emotion-js#973 . It exists only for convenience reasons.

Even if it doesn't matter much - I would prefer using a module and interface augmentation approach over a global one. Also - I'm in the middle of moving those theming APIs into the core package, so maybe it would be better to wait until this is done before proceeding with this one. We highly appreciate your help ❤️

[tomsseisums]

If you want to have different themes [...] and if you are a library [...] developer then you should use a custom theme

But that's what my question is about. With this implementation it is impossible to override the theme, because there aren't exposed generic interfaces any longer. Everything revolves around `Theme.Global`. Or your suggestion is that for such advanced cases, one should roll up their own `ThemeContext` / `ThemeProvider` / `withTheme` / `useTheme` and other handlers? I am aware that this might be answered in your linked post, am in a little rush ATM. Sorry. And thanks for appreciations, we also appreciate your product - I guess that's why we're selflessly investing ourselves into it too! ;)

[Andarist]

Or your suggestion is that for such advanced cases, one should roll up their own ThemeContext / ThemeProvider / withTheme / useTheme and other handlers?

Exactly this, because otherwise, you have to deal with theme clashes etc (it's harder to mix 2 components libraries built on top of emotion). With built-in theme you also can't set its default value (so if you are building a component library with theming you have to make your consumer aware that they need to wrap their app with a ThemeProvider or resolve lack of provided theme manually in your components.

[tomsseisums]

Went over the docs for emotion-js#973 - yeah, makes so much sense! Good share, thanks!

That clears the path for the remainder of concerns I had with the proposed approach here.