Friendlier TypeScript errors. by hasparus · Pull Request #1002 · system-ui/theme-ui

hasparus · 2020-06-15T06:03:52Z

Problem

Hey @wKovacs64 @sbardian, would you have some time to take a look at packages/css/src/types.ts?

Described changes and todos in comments 👇

Already done in DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#45032 Emotion Docs: https://emotion.sh/docs/labels

hasparus · 2020-06-15T06:05:56Z

packages/css/src/types.ts

 }

-interface ColorModesScale extends ColorMode {
+type ColorModesScale = ColorMode & {


modes doesn't match ColorMode index signature (and it shouldn't), so we can't use an interface here.

Property 'modes' of type '{ [k: string]: ColorMode; } | undefined' is not assignable to string index type 'string | string[] | { [K: string]: string | string[] | ...; } | undefined'.

One can still augment ColorMode, so we don't lose anything here.

hasparus · 2020-06-15T06:07:36Z

packages/css/test/errors.ts

@@ -0,0 +1,52 @@
+import { expecter } from 'ts-snippet'


I'm feeling kinda bad installing a library with 16 stars, but no other solution I'm aware of (e.g. tsd, dtslint, dts-jest) integrates with existing Jest setup with no additional config like ts-snippet does.

Eh, the author looks to be very active on GitHub - probably safe (and certainly convenient).

hasparus · 2020-06-15T06:09:00Z

packages/css/src/types.ts

-  | CSSSelectorObject
-  | VariantProperty
-  | UseThemeFunction
+export type ThemeUIStyleObject = ThemeUICSSObject | ThemeDerivedStyles


You can see I removed CSSSelectorObject and merged all objects into ThemeUICSSObject.

The naming is not perfect here.

Instead of CSSSelectorObject, which was causing these really unfriendly errors on all properties, when you made just one typo, we now have CSSOthersObject to handle nested style objects.
As a side-effect, I made style objects even more permissive than I aimed to do, because we know accept CSS properties on all strings.

This is a styling library, the types here are to make work smoother and more effective. not enforce correctness.
I suppose it's better to be too permissive, than confusing.

I hate this (being too permissive) but you're absolutely right that it should aim to be least confusing in the event of an error. I do feel it should enforce correctness but unless we can have both... errors that make sense are probably more valuable than forbidding properties that will just be ignored anyway. 🤷‍♂️

I noticed later today, that this isn't too permissive.
There are variables in CSS 🤦
Arbitrary property names (we can't typecheck the leading dash) with values are 100% legit and useful CSS.

Oh, good point. Then I guess we're set! 😄

hasparus · 2020-06-15T06:16:43Z

packages/css/src/types.ts

    AliasesCSSProperties,
    OverwriteCSSProperties {}

+export type StylePropertyValue<T> =


Any property in a style object is either ResponsiveStyleValue (which includes T), a function which returns such value or a nested style object.

hasparus · 2020-06-15T06:19:41Z

packages/css/src/types.ts

 * For more information see: https://styled-system.com/responsive-styles
 */
-export type ResponsiveStyleValue<T> = T | Array<T | null>
+export type ResponsiveStyleValue<T> = T | Array<T | null | undefined>


I needed it to make @theme-ui/css strict TS, but it makes sense in general. array[index] can always be undefined.

hasparus · 2020-06-15T06:20:24Z

packages/css/src/types.ts

@@ -1,8 +1,3 @@
-/**
- * Copied and adapted from @types/styled-system__css
- * https://github.com/DefinitelyTyped/DefinitelyTyped/blob/028c46f833ffbbb0328a28ae6177923998fcf0cc/types/styled-system__css/index.d.ts


The changes here are pretty noticeable already, I'd say. We probably shouldn't link to "the source" anymore.

hasparus · 2020-06-15T06:27:20Z

packages/css/src/index.ts

-    const value =
-      typeof styles[key] === 'function' ? styles[key](theme) : styles[key]
+  for (const k in styles) {
+    const key = k as keyof typeof styles


A type of key in X isn't always assignable to keys of type of X, even if we forget about the prototype. This part of TypeScript is a bit inconvenient, but it does help with some bugs.

Explanation by Anders Hejlsberg:
microsoft/TypeScript#12253 (comment)

packages/css/src/index.ts

hasparus · 2020-06-15T06:31:56Z

packages/css/src/index.ts


-    if (multiples[prop]) {
-      const dirs = multiples[prop]
+    if (prop in multiples) {


This line is a nice example of how sometimes common JS doesn't translate to strict TypeScript 😞

I'd guess the reasoning is: Since we don't know if prop is a key of multiples, we shouldn't just try to retrieve it without checking. Getters can have side effects!

hasparus · 2020-06-15T06:32:48Z

packages/core/src/index.ts

+function mergeAll<A, B>(a: A, B: B): A & B
+function mergeAll<A, B, C>(a: A, B: B, c: C): A & B & C
+function mergeAll<A, B, C, D>(a: A, B: B, c: C, d: D): A & B & C & D
+function mergeAll<T = Theme>(...args: Partial<T>[]) {


I added a few overloads to infer the type without explicit annotation.

package.json

hasparus · 2020-06-15T06:39:28Z

packages/css/src/types.ts


-export interface UseThemeFunction {
-  (theme: any): Exclude<ThemeUIStyleObject, UseThemeFunction>
+export interface ThemeDerivedStyles {


"use" is already a loaded word in React world. Tell me what you think about the rename.
Another name I considered is ComputedStyle.

Emotion calls it FunctionInterpolation, but since we don't do CSS syntax in tagged template literals here, I'm not sure it would makes sense.

Agreed that UseThemeFunction is not great. I'm good with both ThemeDerivedStyles and ComputedStyles (plural?).

hasparus · 2020-06-15T06:41:07Z

packages/css/src/types.ts

-export interface UseThemeFunction {
-  (theme: any): Exclude<ThemeUIStyleObject, UseThemeFunction>
+export interface ThemeDerivedStyles {
+  (theme: Theme): Exclude<ThemeUIStyleObject, ThemeDerivedStyles>


I added Theme type to the argument here.

This is a small breaking change for TypeScript ppl who added extra properties to their theme object and use them in functions.

I'll draft a TypeScript section in the docs, and describe how to add additional properties to the Theme type.

Hmm, yeah... I have no idea how common of a use case that is, but as long as they still have a way to do it and it's documented, it should be fine.

I have no idea how common of a use case that is

It actually affects half of my projects using Theme UI. 😢

This is also breaking (theme: MyExactTheme) => ... usage, where components depend on exact shape of the theme. This makes sense a lot of sense for blogs, but it's a bug in apps where the theme can be customized by the user.

I have mixed feelings about this change.
~~Possibly, we could add a generic parameter and describe derived styles as <T extends Theme = Theme>(theme: T) => ThemeUICSSObject to allow this 🤔~~

example messy usage of exact theme shape

sx={{ bg: (t: MyTheme) => colorMode === 'dark' ? transparentize(t.colors.modes.dark.primary, 0.2) : darken(t.styles.form.backgroundColor, 0.6) }}

Possibly, we could add a generic parameter and describe derived styles as <T extends Theme = Theme>(theme: T) => ThemeUICSSObject to allow this

That sounds pretty good...?

That sounds pretty good...?

Too good :/ It doesn't work. Just tried it.

Of course. 🤦‍♂️

hasparus · 2020-06-15T07:45:11Z

packages/core/src/index.ts

-// @ts-ignore
 import { css, Theme } from '@theme-ui/css'
-import React from 'react'
+import * as React from 'react'


See facebook/react#18102

It might help with #1000. I didn't test it.

We should probably consider using star import for React everywhere.

See facebook/react#18102

hasparus · 2020-06-15T22:40:07Z

I didn't see where CSSSelectorObject here was being used.

Thanks! I forgot to remove it 🤦

you're way better at TS
you are also better at TS

You gentlemen, are highly misinformed. I have no idea what I'm doing.

hasparus · 2020-06-23T09:29:43Z

Note: Exact themes. This might be a good idea for a PR one day. Ignore this message for now.

TS Playground

// library

declare module 'theme-ui' {
  export interface MyThemeOverride { }

  export type MyTheme = MyThemeOverride extends { theme: infer T } ? T : {};

  export const derivedStyles: (f: (theme: Theme & MyTheme) => unknown) => 'not-implemented'
}

import { Theme } from 'theme-ui';

// userspace

const makeTheme = <T extends Theme>(t: T) => t;

const theme = makeTheme({
  colors: {
    background: 'white',
    text: 'black',
    blue: {
      light: '#187abf',
      dark: '#235a97'
    }
  }
})

type ExactTheme = typeof theme;

declare module 'theme-ui' {
  export interface MyThemeOverride {
    theme: ExactTheme
  }
}

import { derivedStyles } from 'theme-ui';

// when your theme is not dynamic, knowledge about its exact shape is  convenient
derivedStyles(t => t.colors.blue.light);

// example

const lightBlue1 = theme.colors.blue.light;

const genericTheme: Theme = theme;

// multiple errors
const lightBlueError = genericTheme.colors.blue.light

hasparus · 2020-06-23T21:09:35Z

Okay, this PR is probably finished if the page I added to the docs makes sense (@jxnblk)

The TS chapter for the docs is here: https://deploy-preview-1002--theme-ui.netlify.app/guides/typescript/

I'm not sure if it should be in guides or below Migrating.
If it stays in the guides, I'll add a link to it to the Guides page.

mxstbr

This looks excellent, and I also really like the docs page. I think I would move the "Exact theme" explanation further up, as I think we want that to be more common use of theme (vs. using .get())?

hasparus · 2020-06-24T18:26:37Z

Thanks!

Based on a sample of size two of my own projects (serious statistics), I used exact theme type 3 times.

While passing colors to zdog's imperative API.
In variant prop of custom Link component.
In a Stack component, because I couldn't index theme.space with number (I changed ObjectOrArray in this PR, it should be okay right now.)

And I found get or optional chaining used few dozen times.

What's important, I broke following scenario

css({ color: (theme: ExactTheme) => theme.colors.blue[300] })

in this PR. It worked previously, because theme here was typed as any. There is a small trade-off.

	`(theme: any) => somevalue`	`(theme: Theme) => somevalue`
JS user	😢 no code-completion	😄 completion of scale names
TS user	😢 forced explicit annotation 😄 easy to annotate with exact theme	😄 no need for annotation 😄 interoperable by default 😢 exact theme requires assertion (`as ExactTheme`)

There's a possible fix few comments above.

I'll move ExactTheme example to the top and add this. This should be pretty useful.

import { ContextValue } from 'theme-ui';

interface ThemeCtx extends Omit<ContextValue, "theme"> {
  theme: ExactTheme;
}

export const useTheme = (useThemeUI as unknown) as () => ThemeCtx;

hasparus · 2020-06-25T08:09:52Z

Okay, I added exact theme hook example and move it up.

I'm not sure if I should explain the motivation for it in the docs. I don't want to make them bloated.
(TS doesn't know Theme UI will always give you your theme from the context,
so it will always behave like you get any theme, what makes sense for huge apps and libraries.)

mxstbr

This looks fantastic to me and will be such a huge improvement for our TS users. @jxnblk any further thoughts?

jxnblk

I'm mostly trusting those of you more familiar with TS here, but left some comments about the docs.

Ultimately (not blocking), I think we should point out the differences between usage in TS and JS throughout the docs where relevant, but think this is a good first step and a way to consolidate TS-related things in a single place

packages/docs/src/pages/guides/typescript.mdx

jxnblk · 2020-06-30T16:53:03Z

packages/docs/src/pages/guides/typescript.mdx

+const lightBlue = theme.colors.blue.light
+```
+
+You can then reexport `useThemeUI` hook with narrowed type.


For clarification, is ExactType only used in this instance?

Not exactly sure what if I understand you here, but yes?: Theme UI types don't know about user's theme exact shape. Everything will just behave like we're writing a bunch of reusable components for any theme.

This can be improved, I think. We could "tell Theme UI about our theme" like in Overmind: https://overmindjs.org/core/typescript#1-declare-module. I only have a proof of concept of this code in one of the comments here.

Was mostly for my own clarification, and I made a typo, but it sounds like the ExactTheme type is only needed when you're using the custom useThemeUI hook, right? I.e. I wouldn't use that type anywhere else?

This can be improved, I think.

Yeah, totally -- I understand that is outside the scope of this PR

it sounds like the ExactTheme type is only needed when you're using the custom useThemeUI hook, right? I.e. I wouldn't use that type anywhere else?

ATM that's correct.
I just made #1090 so we can "turn on" using this type everywhere else.

jxnblk

hasparus added 4 commits June 14, 2020 14:08

Add label prop to ThemeUIStyleObject

7870a45

Already done in DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#45032 Emotion Docs: https://emotion.sh/docs/labels

Change core/test/index extension to tsx

0647c3c

Add overloads to merge.all

8e65bd9

Fix type errors in core/test/index

41e2e63

hasparus commented Jun 15, 2020

View reviewed changes

packages/css/src/index.ts Outdated Show resolved Hide resolved

hasparus commented Jun 15, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

hasparus marked this pull request as draft June 15, 2020 06:37

hasparus commented Jun 15, 2020

View reviewed changes

hasparus force-pushed the better-ts-errors branch from 96c707d to 624c001 Compare June 15, 2020 08:00

hasparus added 7 commits June 15, 2020 10:22

Bump deps to use optional chaining in tests

671c4e7

Use optional chaining in @theme-ui/css tests

089b302

Report friendlier TypeScript errors in style objects

103d6b9

Make @theme-ui/css strict TypeScript

ea8346f

Test that ThemeUIStyleObject errors are not confusing anymore

8cfdfad

Update reexport name

bebb666

Import * as React in @theme-ui/core

120192f

See facebook/react#18102

hasparus force-pushed the better-ts-errors branch from 624c001 to 120192f Compare June 15, 2020 08:23

hasparus marked this pull request as ready for review June 15, 2020 08:46

hasparus requested a review from mxstbr June 15, 2020 09:04

sbardian approved these changes Jun 16, 2020

View reviewed changes

hasparus mentioned this pull request Jun 17, 2020

Convert to TypeScript #668

Closed

32 tasks

hasparus linked an issue Jun 17, 2020 that may be closed by this pull request

Type Emotion label property in @theme-ui/css. #952

Closed

hasparus added 2 commits June 18, 2020 00:17

Make test/errors.ts expectSnippet strict

30c522f

Export ColorModesScale

89253aa

hasparus force-pushed the better-ts-errors branch from 80f6453 to c9da2e1 Compare June 23, 2020 20:39

hasparus added 2 commits June 23, 2020 23:01

Draft TypeScript chapter for the docs

8cfd2ef

Add numeric index signature to ObjectOrArray

00ff818

hasparus force-pushed the better-ts-errors branch from c9da2e1 to 00ff818 Compare June 23, 2020 21:01

Remove unused imports

a5ad143

mxstbr reviewed Jun 24, 2020

View reviewed changes

Add exact theme reexport example

9033499

hasparus added 3 commits June 28, 2020 13:32

Reference other pages in docs and add a sentence

3836ea1

Merge remote-tracking branch 'origin/master' into better-ts-errors

92b13ec

Rename ObjectOrArray to Scale

26017d4

This was referenced Jun 28, 2020

@theme-ui/typography => Typescript #890

Merged

(@theme-ui/mdx) adopt TypeScript #1031

Merged

mxstbr approved these changes Jun 30, 2020

View reviewed changes

jxnblk reviewed Jun 30, 2020

View reviewed changes

hasparus added 2 commits June 30, 2020 21:06

Steal jxnblk's paragraph

b4a883f

Update typescript.mdx

0756514

jxnblk approved these changes Jun 30, 2020

View reviewed changes

hasparus merged commit c4a59c7 into system-ui:master Jun 30, 2020

hasparus deleted the better-ts-errors branch July 26, 2020 19:46

hasparus mentioned this pull request Aug 10, 2020

Low precision TypeScript error in SystemStyleObject #764

Closed

Conversation

hasparus commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparus Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hasparus Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparus Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparus Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparus Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasparus commented Jun 15, 2020

Uh oh!

hasparus commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasparus commented Jun 23, 2020

Uh oh!

mxstbr left a comment

Choose a reason for hiding this comment

Uh oh!

hasparus commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasparus commented Jun 15, 2020 •

edited

Loading

hasparus Jun 15, 2020 •

edited

Loading

hasparus Jun 15, 2020 •

edited

Loading

hasparus Jun 15, 2020 •

edited

Loading

hasparus Jun 15, 2020 •

edited

Loading

hasparus Jun 17, 2020 •

edited

Loading

hasparus commented Jun 23, 2020 •

edited

Loading

hasparus commented Jun 24, 2020 •

edited

Loading

hasparus commented Jun 25, 2020 •

edited

Loading

hasparus Jul 26, 2020 •

edited

Loading