Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several TypeScript errors when upgrading to 0.16.1 #2456

Closed
matt-koevort opened this issue Sep 14, 2023 · 5 comments · Fixed by #2457
Closed

Several TypeScript errors when upgrading to 0.16.1 #2456

matt-koevort opened this issue Sep 14, 2023 · 5 comments · Fixed by #2457
Assignees
Labels
bug Something isn't working

Comments

@matt-koevort
Copy link

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
Copy link
Member

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
Copy link
Member

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
        }}
      />
    </>
  );
}

@hasparus hasparus self-assigned this Sep 22, 2023
@hasparus hasparus added the bug Something isn't working label Sep 22, 2023
@matt-koevort
Copy link
Author

matt-koevort commented Sep 22, 2023

@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
Copy link
Author

matt-koevort commented Sep 22, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants