system-ui / theme-ui

Build consistent, themeable React apps based on constraint-based design principles
https://theme-ui.com
MIT License
5.28k stars 676 forks source link

Several TypeScript errors when upgrading to 0.16.1 #2456

Closed matt-koevort closed 7 months ago

matt-koevort commented 1 year ago

Describe the bug When upgrading to 0.16.1, we have introduced a bunch of TypeScript errors. An example of one such error, is when trying to access a string key on a Scale type property (e.g theme.shadow). An array index works as expected without any TS errors, but when trying to access a key (our shadow property in the theme is an object, not an array) we see a TS error.

This can be solved with type assertions, but it would be quite cumbersome to update all instances like this.

E.g

<div
  sx={{
    boxShadow: (theme) => theme.shadows.firstLevel
  }}
/>
Property 'firstLevel' does not exist on type 'Scale<BoxShadow>'.
  Property 'firstLevel' does not exist on type 'BoxShadow[]'.ts(2339)

To Reproduce Steps to reproduce the behavior:

  1. Clone this CRA sandbox
  2. Open the project in a TypeScript enabled editor, or run tsc --noEmit (for example)
  3. Note error when trying to access a string key on theme.shadows (as opposed to a numbered index)

Expected behavior The type Scale indicates that it can be an array or an object. Out of the box it seems to assume that any property of type Scale is an array.

Screenshots

image

image

Additional context Add any other context about the problem here.

hasparus commented 12 months ago

Thanks for the issue, and the reproduction @matt-koevort.

It's interesting, because the error shows up on a scale read from the theme, but not if it's a standalone variable of the same type.

image

Looking into it!

hasparus commented 11 months ago

Okay, it seems that we can no longer read string keys from an union of array and object with string index signature.

This is a bit of a tricky case then, because we need to keep accepting an array OR a dict in the theme user gives to the theme provider, but give out a broader type in sx prop so it can be read safely. I'm not sure if a bugfix is even possible, but I have an idea for a feature that would solve this and give you a better, stricter type.

I'm curious why does it work as expected in my commercial production project which is using the same versions. Do you know which specific version update of either Theme UI or TypeScript caused this issue?

In the meantime, may I interest you in using makeTheme and an assertion for a workaround? It's not ideal, but you'll get more safety & autocomplete.

/** @jsxImportSource theme-ui */

import { makeTheme } from "@theme-ui/css/utils";

const theme = makeTheme({
  shadows: {
    firstLevel: "0 0 4px 2px rgba(0, 0, 0, .125)",
  },
});

type MyTheme = typeof theme;

function App() {
  return (
    <>
      <div
        sx={{
          boxShadow: (theme) => (theme as MyTheme).shadows![0], // errors as expected
        }}
      />
      <div
        sx={{
          boxShadow: (theme) => (theme as MyTheme).shadows.firstLevel, // this works, and firstLevel gets autocompleted
        }}
      />
    </>
  );
}
matt-koevort commented 11 months ago

@hasparus thanks for looking into this. To be honest I had a fiddle with this in TypeScript and also found it difficult to achieve the broader type as you mentioned.

The only problem we would have with your suggestion, is that (a) we have far too many instances of the theme callback so updating all of them would be a bit of a headache, and (b) we don't actually have the specific typing on the theme as this is a library that is used in another application which actually sets the theme via the ThemeProvider.

It doesn't seem to be a typescript (v5.2.2) upgrade that is causing this, but rather the upgrade to @theme-ui/core@0.16.1

matt-koevort commented 11 months ago

We do have our own type definition for the theme, but it does not comply with the shape of the Theme definition in theme-ui. So the only way we can use assertions is to do something like:

boxShadow: (theme) => (theme as unknown as OurThemeDefinition).shadows.elevationElevated,

which we would probably need to do in about 50+ places, and is something to maintain going forward, so I don't think it's a great solution.

hasparus commented 11 months ago

It puzzles me because I downgraded to 0.15, TS 4 and old @types/react, and I could still see the error in your repro, while I don't have it in my prod project.

Anyway, try this ⬇️

https://github.com/matt-koevort/react-theme-ui/compare/main...hasparus:theme-ui-0.16-type-error-repro:main

Unfortunately, it only works at the top-level theme if you pass a function to sx, because swapping the theme type for StyledPropertyValues (like boxShadow) added 21 seconds of typechecking time to Theme UI repo, so it definitely would have an effect on user build times.