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

Chore: Update @stitches/react #461

Merged
merged 2 commits into from
Feb 1, 2023
Merged

Conversation

thomasdigby
Copy link
Member

@thomasdigby thomasdigby commented Jan 30, 2023

Updating @stitches/react to fix a number of issues with the library with later versions of Typescript.

@Mhoog and I have been through the wringer over the past few weeks trying to resolve some type issues with @atom-learning/components and @stitches in particular, resulting in this need to update @stitches to the latest published version (a beta).

I've written some of the steps that we went through to provide a bit of context, but it included a fair amount of trial and error testing, patching local versions of the packages, whilst also trying to resolve local TS issues with our codebase, so please let me know if you want any more detail on why we did certain things, but hopefully, we concluded with a relatively simple fix.

The initial issue we hit was with how @stitches accessed types from modules that weren't exported from the module

Exported variable "Variable" has or is using name 'Stitches' from external module

This was flagged sporadically across styled() and standard Box, Flex, etc. components, and we've also had this a couple of times in our own codebase. @Mhoog fixed this in #454

The next issue was types missing altogether from @atom-learning/components & @stitches, which required a change to TS module resolution "node16" to "node", alongside an upgrade to TS itself. There are a whole heap of weird compatibility issues with the various ways that TS resolves modules and the requirements that it has with each condition to return the correct files. So TS needed an update and the type imports needed to include the relevant extension. This was fixed in this PR to @stitches stitchesjs/stitches#1084

The final issue, now we'd upgraded to TS 4.9 was with @stitches/reacts use of symbols. stitchesjs/stitches#1082

TS4118: The type of this node cannot be serialized because its property '[$$PropertyValue]' cannot be serialized.

Thankfully that fix went into the beta release, so the only remaining issue was our use of inset as a key in our @stitches utils 🤷

Type 'CSS | undefined' is not assignable to type 'CSS<{ sm: string; md: string; lg: string; xl: string; reducedMotion: string; allowMotion: string; hover: string; }, { col...

For some reason, in this updated version of @stitches, the key inset produced this TS error. I can only assume that somewhere in the update they've changed how custom utils and standard CSS properties overlap, and produce an error if any key in utils matches a standard CSS property. https://developer.mozilla.org/en-US/docs/Web/CSS/inset

We don't use inset too much in the codebase, and unfortunately, we can't use it natively until we drop Safari 12/13, so I have just removed the custom util and replaced it inline with top: 0; right: 0; bottom: 0; left: 0.

I've probably forgotten a few things to mention, but that's broadly how we got here.

@thomasdigby thomasdigby marked this pull request as ready for review February 1, 2023 09:42
Copy link
Member

@Mhoog Mhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR description brings back some traumas.. 😅

lib/package.json Show resolved Hide resolved
@thomasdigby thomasdigby merged commit fb4c47a into main Feb 1, 2023
@thomasdigby thomasdigby deleted the chore/upgrade-stitches-1.3.1 branch February 1, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants