Updated types to support global Theme definition #1609

tomsseisums · 2019-11-05T20:50:59Z

What:
Fixes #1197

Why:
The current typings do not support a way of providing theme types globally. The re-exports can only go so far, and the styled themed re-export actually tampers with babel plugin.

How:
I added Theme export via a global namespace as suggested by @JakeGinnivan so that it can be overriden with:

// emotion-theme.d.ts
declare global {
  namespace Emotion {
    export interface Theme {
      your: 'theme',
      comes: 'here'
    }
  }
}

And updated all usages of previous default any to use the namespace one.

Checklist:

Documentation
Tests
Code complete
Changeset

I don't know how to add test cases for this.
I tried by adding a globalTests.tsx containing actual tests and emotion-theme.d.ts containing the declaration alongside the respective tests. But all my efforts to prevent emotion-theme.d.ts global declaration from bleeding into un-overriden usecase tests failed. If the global theme declaration bleeds into un-overriden usecase tests, it breaks the tests there. Probably for the same reasons as can be seen in styled-components typings:

/**
 * This interface can be augmented by users to add types to `styled-components`' default theme
 * without needing to reexport `ThemedStyledComponentsModule`.
 */
// Unfortunately, there is no way to write tests for this
// as any augmentation will break the tests for the default case (not augmented).
// tslint:disable-next-line:no-empty-interface
export interface DefaultTheme {}

~~I'm opening this as a draft because I lack documentation/changeset, and want some initial reviews.~~
Updated to ready, because... how would one review it otherwise?

Also, what does Code complete mean in the checklist?

changeset-bot · 2019-11-05T20:51:01Z

🦋 Changeset is good to go

Latest commit: f391e6c

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

packages/core/types/index.d.ts

JakeGinnivan · 2019-11-11T02:01:50Z

More general question.

Why don't we just remove the Theme generic parameter from everywhere and just reference Emotion.Theme or the module version directly?

It would remove a heap of generic params and complexity from the types?

JakeGinnivan · 2019-11-11T02:23:40Z

Nice work getting the ball rolling on this, I think this will be an awesome change. I had a go at just removing the generic params for theme from everywhere?

Thoughts:

tomsseisums#1

Also, for compat we could just use documentation to fill the gap (I thought I posted this somewhere, but can't find it. Oh well).

declare global {
  export interface Theme extends Record<string, any> {
  }
}

JakeGinnivan · 2019-11-11T02:30:15Z

After doing the above, I wonder if there is much value in the emotion-theming package at all.

IMO v11 could completely remove it, you just move the ThemeProvider, hook and HoC into core?

emmatown · 2019-11-11T10:50:51Z

I'm good with moving the theming APIs to core. @Andarist want to create a PR?

Andarist · 2019-11-11T10:55:34Z

@mitchellhamilton sure, gonna handle this today.

Andarist · 2019-11-18T22:09:16Z

@joltmode I've just merged in the PR which has moved theming APIs into @emotion/core. Could you rebase this branch and incorporate suggestions from tomsseisums#1 ?

Andarist · 2019-11-22T14:54:08Z

@joltmode will you be able to work on this in the near future? If not - that's perfectly fine, we'd only appreciate a heads up so somebody could take over this.

tomsseisums · 2019-11-22T17:29:45Z

@Andarist yes, will strive to push this forward during the weekend.

- [side-effect] Updated back to 2 space indentation

codesandbox-ci · 2019-11-23T20:52:15Z

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 f391e6c:

Sandbox	Source
Emotion	Configuration

Andarist · 2019-11-23T21:06:17Z

Looking quite good!

As discussed briefly before - if it's possible we'd prefer to keep that Emotion.Theme as part of the @emotion/core module - rather than in the global namespace.

tomsseisums · 2019-11-23T21:10:26Z

Yup, looking to update that one.

Currently working on fixing tests, forgot to run before pushing.

Was a pretty nasty rebasing, loads of changes inbetween! Quite possibly this was the toughest merge conflict in my development life so far. 🥇

- Added TODO for one possible issue

packages/styled/types/tests.tsx

tomsseisums · 2019-11-23T21:55:40Z

docs/typescript.mdx

@@ -248,34 +248,33 @@ const App = () => (
 ### Define a Theme


The whole docs on Theme usage with TS feels massively outdated now, and I feel like it should be reworded altogether.

Exposing for discussion.

Right - I feel like we can handle it in a separate PR though. Ideally, such docs would present the current solution with its tradeoffs and incorporate stuff from this #973

If it's not in this PR, it should definitely be a blocker for releasing v11.

Yes, this is crucial and I plan to get this done before v11 👍

packages/core/types/tests-theming.tsx

Andarist · 2019-11-23T22:32:47Z

packages/serialize/types/index.d.ts

  (mergedProps: MergedProps): Interpolation<MergedProps>
 }

-export type Interpolation<MergedProps = undefined> =
+export type Interpolation<MergedProps = { theme: Theme }> =


actually I think that Interpolation is also used in css definition and in there you don't have access to Theme, so the previous version with default being undefined was correct for css's case

Yeah, this and the serializeStyles stuff should be better viewed by @JakeGinnivan. Not on the same train of thought as he is on with these.

I also invited you @JakeGinnivan as a member to my repo, so it's not mandatory to have to PR.

I kinda backed away from these types during my initial work. I wonder if we should just not have a default? (then if not specified it will be unknown)

css definition and in there you don't have access to Theme

Just for my understanding, why is this the case? Looking at the other types elsewhere we could actually do another round of simplification if we could add theme into these types directly

css definition and in there you don't have access to Theme

Just for my understanding, why is this the case?

I've meant css function - you can't do css(() => /**/). From what remember such function just gets stringified. css call is being executed without any notion of context, you can hoist it wherever you want etc - so it has no access to theme or props.

A different story is the css props - where you can get access to the builtin Theme.

Looking at the other types elsewhere we could actually do another round of simplification if we could add theme into these types directly

What kind of simplification do you have in mind? I've noticed now that we have Interpolation, CSSInterpolation and even InterpolationWithTheme and I think they might be used sometimes in wrong contexts (especially the Interpolation type). I need to draw out the dependency tree of those types somewhere for myself to analyze this further 😅

Andarist · 2019-11-23T22:33:57Z

packages/serialize/types/index.d.ts

@@ -79,7 +83,7 @@ export type Interpolation<MergedProps = undefined> =
  | ObjectInterpolation<MergedProps>
  | FunctionInterpolation<MergedProps>

-export function serializeStyles<MP>(
+export function serializeStyles<MP = { theme: Theme }>(


same here - serializeStyles gets called by css

Andarist · 2019-11-23T22:36:59Z

@JakeGinnivan would appreciate you taking a look at this as well

- Add test case for fully overriding theme

JakeGinnivan · 2019-11-25T00:38:33Z

docs/typescript.mdx

@@ -248,34 +248,33 @@ const App = () => (
 ### Define a Theme


If it's not in this PR, it should definitely be a blocker for releasing v11.

docs/typescript.mdx

JakeGinnivan · 2019-11-25T00:51:45Z

packages/serialize/types/index.d.ts

  (mergedProps: MergedProps): Interpolation<MergedProps>
 }

-export type Interpolation<MergedProps = undefined> =
+export type Interpolation<MergedProps = { theme: Theme }> =


I kinda backed away from these types during my initial work. I wonder if we should just not have a default? (then if not specified it will be unknown)

JakeGinnivan · 2019-11-25T00:58:27Z

packages/styled/types/base.d.ts

+        ComponentProps &
+          SpecificComponentProps &
+          StyleProps &
+          AdditionalProps & { theme: Theme }


Is the union with theme needed here? I think the entrypoint (the styled function) adds theme?

{ theme: Theme } gets always passed in as StyleProps, so we should be able to remove this from here, but when I try to do that then I get such errors:

Error: /emotion/packages/styled/types/tests-base.tsx:156:32 ERROR: 156:32 expect TypeScript@next compile error: Object is possibly 'undefined'. /emotion/packages/styled/types/tests.tsx:19:7 ERROR: 19:7 expect Expected type to be: { theme?: Theme | undefined; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { bar: string; } & { theme: Theme; } got: { theme?: Theme | undefined; } & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & { bar: string; }

Still investigating this, but right now I don't see why this fails.

While I don't understand why this fails - it might actually be a proper thing to specify this here instead of passing it from the above level in StyleProps. After all - theme is always available in interpolations, so it shouldn't be possible to "cancel" it out by passing StyleProps without it?

Ok - I know why this thing has been failing:
https://github.com/joltmode/emotion/blob/464922628c9197988ac7ae38d33b84b6eb6d63d8/packages/styled/types/base.d.ts#L89-L93
This doesn't include StyleProps. It should right?

Regardless of that - my previous question stands. Should we pass Theme as part of StyleProps from the above or should we just pass it directly to Interpolation?

Ok, I've decided to move { theme: Theme } inline where parameters are being passed to Interpolation inside styled components. Seems the most appropriate solution - but I'm happy to discuss this further, maybe I've missed something?

More than that - I'm right now kinda questioning if we need StyleProps at all, what do you think?

JakeGinnivan · 2019-11-25T00:59:42Z

packages/serialize/types/index.d.ts

  (mergedProps: MergedProps): Interpolation<MergedProps>
 }

-export type Interpolation<MergedProps = undefined> =
+export type Interpolation<MergedProps = { theme: Theme }> =


css definition and in there you don't have access to Theme

Just for my understanding, why is this the case? Looking at the other types elsewhere we could actually do another round of simplification if we could add theme into these types directly

… StyleProps)

stramel · 2019-12-20T20:23:33Z

@Andarist I'm not sure if this is causing a problem or if it's just misleading.

I noticed it seems to be pulling a test base typing instead the interface from the main library.

Andarist · 2019-12-20T21:33:33Z

Interesting, could you prepare a runnable repro case (a simple repository)? That would help me a lot to jump into investigating this. It might be also worth reporting an issue about this. Notice though that main library typing is picked up correctly (you can see that on the right of your screenshot - first item on the list is the one from the main library), the problem is though that the test typings are also picked up.

stramel · 2019-12-21T04:07:13Z

I will do my best to create a repo

JakeGinnivan · 2019-12-27T02:43:57Z

I wonder if we should just not include the test files @Andarist?

Something like this

Andarist · 2019-12-27T09:08:42Z

Yeah, that would be the simplest fix. Prepared a PR for that - #1702

…ed by consumers (emotion-js#1609) * Updated types to support global Theme definition * Updated documentation to reflect new theme typings * Simplified MuiTheme import syntax in docs * Use previous theme declaration in docs - [side-effect] Updated back to 2 space indentation * Update Button.tsx import declaration for styled * Updated type definitions to default to any if not defined * Quick pass at removing theme generic param * Fixed issue with Global * Fixed most of the test issues - Added TODO for one possible issue * Updated global Theme type declaration to module specific * Removed exports from test files * Use the incomplete theme prop testcase - Add test case for fully overriding theme * Added note about where styled tests get their theme declaration from * tweak docs * Remove InterpolationWithTheme type * Move adding Theme to Styled interpolations inline (and not as part of StyleProps) * Cleanup Interpolation-related types * Add changeset

tomsseisums marked this pull request as ready for review November 5, 2019 21:05

tomsseisums commented Nov 5, 2019

View reviewed changes

packages/core/types/index.d.ts Outdated Show resolved Hide resolved

JakeGinnivan reviewed Nov 11, 2019

View reviewed changes

packages/core/types/index.d.ts Outdated Show resolved Hide resolved

tomsseisums mentioned this pull request Nov 11, 2019

Quick pass at removing theme generic param tomsseisums/emotion#1

Merged

Andarist mentioned this pull request Nov 15, 2019

adding types for @emotion/native #1634

Merged

4 tasks

Toms Seisums and others added 8 commits November 23, 2019 22:28

Updated types to support global Theme definition

07a70a4

Updated documentation to reflect new theme typings

3ad510f

Simplified MuiTheme import syntax in docs

c55d448

Use previous theme declaration in docs

7be6f64

- [side-effect] Updated back to 2 space indentation

Update Button.tsx import declaration for styled

353dc2e

Updated type definitions to default to any if not defined

358c12d

Quick pass at removing theme generic param

23717d4

Fixed issue with Global

36ea5ee

tomsseisums force-pushed the topics/global-theme-type branch from 16ae3b7 to 36ea5ee Compare November 23, 2019 20:50

tomsseisums added 2 commits November 23, 2019 23:30

Fixed most of the test issues

1c0d7c3

- Added TODO for one possible issue

Updated global Theme type declaration to module specific

84bab57

tomsseisums commented Nov 23, 2019

View reviewed changes

packages/styled/types/tests.tsx Show resolved Hide resolved

tomsseisums commented Nov 23, 2019

View reviewed changes

Andarist reviewed Nov 23, 2019

View reviewed changes

packages/core/types/tests-theming.tsx Show resolved Hide resolved

Andarist reviewed Nov 23, 2019

View reviewed changes

tomsseisums added 3 commits November 24, 2019 15:09

Removed exports from test files

44adafb

Use the incomplete theme prop testcase

2e8b12b

- Add test case for fully overriding theme

Added note about where styled tests get their theme declaration from

bef05bd

Andarist mentioned this pull request Nov 24, 2019

Accept custom context in ThemeProvider #1650

Closed

JakeGinnivan reviewed Nov 25, 2019

View reviewed changes

Andarist mentioned this pull request Nov 26, 2019

Types: Missing overload for default export from @emotion/styled ? #1640

Open

Andarist mentioned this pull request Dec 3, 2019

[SUPPORT] Composing dynamic styles + Themes + Typescript #1670

Closed

Andarist added 3 commits December 3, 2019 23:37

Merge branch 'next' into topics/global-theme-type

2025fa2

tweak docs

f9ff2b2

Remove InterpolationWithTheme type

4649226

Andarist mentioned this pull request Dec 4, 2019

@emotion/styled with theme is still marked as any and not the custom theme #1671

Closed

Andarist added 3 commits December 7, 2019 17:04

Move adding Theme to Styled interpolations inline (and not as part of…

94d0d1f

… StyleProps)

Cleanup Interpolation-related types

c2fb7f1

Add changeset

f391e6c

Andarist merged commit c643107 into emotion-js:next Dec 7, 2019

Andarist deleted the topics/global-theme-type branch December 7, 2019 21:03

This was referenced Dec 7, 2019

[TypeScript] Leverage declaration merging for themes #1507

Closed

Typescript support for theme when using css prop #1197

Closed

github-actions bot mentioned this pull request Nov 10, 2020

Version Packages #2046

Merged

jtbandes mentioned this pull request Nov 13, 2021

Ability to override/merge custom interface with Theme declaration microsoft/fluentui#20513

Closed

Updated types to support global Theme definition #1609

Updated types to support global Theme definition #1609

Conversation

tomsseisums commented Nov 5, 2019 • edited Loading

changeset-bot bot commented Nov 5, 2019 • edited Loading

🦋 Changeset is good to go

JakeGinnivan commented Nov 11, 2019

JakeGinnivan commented Nov 11, 2019

JakeGinnivan commented Nov 11, 2019

emmatown commented Nov 11, 2019

Andarist commented Nov 11, 2019

Andarist commented Nov 18, 2019

Andarist commented Nov 22, 2019

tomsseisums commented Nov 22, 2019

codesandbox-ci bot commented Nov 23, 2019 • edited Loading

Andarist commented Nov 23, 2019

tomsseisums commented Nov 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Nov 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stramel commented Dec 20, 2019

Andarist commented Dec 20, 2019

stramel commented Dec 21, 2019

JakeGinnivan commented Dec 27, 2019

Andarist commented Dec 27, 2019

tomsseisums commented Nov 5, 2019 •

edited

Loading

changeset-bot bot commented Nov 5, 2019 •

edited

Loading

codesandbox-ci bot commented Nov 23, 2019 •

edited

Loading