nandorojo / dripsy

🍷 Responsive, unstyled UI primitives for React Native + Web.
https://dripsy.xyz
MIT License
1.98k stars 77 forks source link

onlyAllowThemeValues: "always" not working #264

Closed jaivinwylde closed 1 year ago

jaivinwylde commented 1 year ago

With setting onlyAllowThemeValues to always, I would expect the following not to be allowed (theme fontSizes aren't numbers)

sx={{ fontSize: 10 }}

Dripsy allows this though, and the fontSize type is this when it should only be the theme keys:

(property) fontSize?: "0" | "1" | "2" | "3" | "4" | "5" | (number | null | undefined)[] | (number & {}) | ("0" | "1" | "2" | "3" | "4" | "5" | null)[] | ((theme: DripsyFinalTheme) => (ResponsiveValue<number | undefined> & {}) | ResponsiveValue<...>) | null | undefined

I used fontSize as the only example, but the same happens for every style prop.

(An example of what the type should look like from the docs) image

I'm using TypeScript 4.8.2 and Dripsy 3.8.1 - with these versions, intellisense is working, but strict theme values are not (same behavior on resolution ~4.4)

nandorojo commented 1 year ago

Can I see your theme & types setup?

jaivinwylde commented 1 year ago
import { makeTheme } from "dripsy"

const sizes = { "0": 0, "1": 8, "2": 16, "3": 24, "4": 32, "5": 40, "6": 48 }
export const fontName = "lexend"

export const theme = makeTheme({
  colors: {
    background: "#121212",
    secondary: "#202020",
    highlight: "#353535",

    text: "#ffffff",
    muted: "#888888",

    accent: "#4fa4d3",
    success: "#6bc754",
    warning: "#f5ba47",
    danger: "#c54b4b",
  },

  customFonts: {
    [fontName]: {
      default: fontName,
      400: fontName,
      500: "Lexend-Medium",
      600: "Lexend-SemiBold",
      700: "Lexend-Bold",
    },
  },
  fonts: {
    root: fontName,
  },
  fontSizes: { "0": 13, "1": 16, "2": 19, "3": 24, "4": 32, "5": 40 },
  fontWeights: { normal: "400", medium: "500", bold: "600", black: "700" },

  breakpoints: ["425px", "768px", "1024px", "1200px"],
  zIndices: { "0": 0, "1": 100, "2": 200, "3": 300, "4": 400 },
  radii: { sm: 5.6, md: 8, lg: 16 },
  sizes,
  space: sizes,

  // Variants
  text: {
    body: {
      fontFamily: "root",
      fontWeight: "normal",
      color: "text",
    },
  },

  types: {
    onlyAllowThemeValues: "always",
    reactNativeTypesOnly: true,
  },
})

type Theme = typeof theme

declare module "dripsy" {
  // eslint-disable-next-line @typescript-eslint/no-empty-interface
  interface DripsyCustomTheme extends Theme {}
}
nandorojo commented 1 year ago

Can you add as const to the sizes variable?

jaivinwylde commented 1 year ago

Just tested that and the same thing happens 🤔

image

nandorojo commented 1 year ago

can you try dripsy 3.6 and typescript 4.4?

jaivinwylde commented 1 year ago

Tried and also doesn't seem to work.

Dripsy version: image

Typescript resolution: image

This still happens: image

nandorojo commented 1 year ago

I’ll have to take a look when I have time. The TS setup is super complicated to maintain, I may have to rewrite it at some point

nandorojo commented 1 year ago

What if you set your theme keys to numbers with a dollar sign $0 instead of using an array, as recommended in the docs?

jaivinwylde commented 1 year ago

This with TS 4.9.4 image

This with TS 4.4 & Dripsy 3.6 image

awinograd commented 1 year ago

The below is using typescript 4.7.x

I am getting started learning about dripsy and came across this issue too. Here are some of my findings so far. Standard disclaimer that any of my claims below could be incorrect

First I started looking at OnlyAllowThemeValueForKey. I noticed that keyof DripsyThemeWithoutIgnoredKeys was returning never.

EDIT: Below quoted section is no longer applicable. The template installed dripsy 3.8.x but the module augmentation specified in the docs appears to work with 3.10.x.

That lead me to update the path for module augmentation to target the file in which DripsyCustomTheme is initially declared

import '@dripsy/core/src/declarations';
declare module '@dripsy/core/src/declarations' {
  interface DripsyCustomTheme extends MyTheme {}
}

EDIT After continuing forward with the above declaration, I ended up having to augment more than one path... I think this started breaking again for me when I added @dripsy/gradient but i'm not sure.

// The exact module we need to update appears to change randomly so let's just tackle all possible
// paths we may need to augment
import '@dripsy/core/src/declarations';
import '@dripsy/core/build/declarations';
declare module '@dripsy/core/build/declarations' {
  interface DripsyCustomTheme extends MyTheme {}
}
declare module '@dripsy/core/src/declarations' {
  interface DripsyCustomTheme extends MyTheme {}
}
declare module 'dripsy' {
  interface DripsyCustomTheme extends MyTheme {}
}

/EDIT

Then, typescript started properly returning the expected keys for keyof DripsyThemeWithoutIgnoredKeys. After that was resolved, I saw the following errors in OnlyAllowThemeValueForKey

AliasedScale extends Scale = AliasToScale<Scale>

Type 'AliasToScale' does not satisfy the constraint 'Scale'. 'AliasToScale' is assignable to the constraint of type 'Scale', but 'Scale' could be instantiated with a different subtype of constraint

I dont think you actually want extends Scale in the line above, because you actually want a narrower type for AliasedScale, namely all of the full-length keys WITHOUT the abbreviated values. ** This is dripsy specific terminology so please 2x check me here. Perhaps you meant extends ScaleValue?

Once extends Scale is removed the remaining error is

`NonNullable< DripsyFinalTheme['types']

['onlyAllowThemeValues'][AliasedScale] extends 'always'`

Type 'AliasedScale' cannot be used to index type '"always"'.

My understanding is that the previous ternary with the condition extends "always" narrows the type inside the "true" case of the ternary, but doesn't conversely narrow for the false case such that you're only left with the object type. I think the solution is to add an additional ternary condition to ensure you've got an object at this point. I did get stuck here, however, and don't have a suggested edit.

For a new project, that level of granularity isn't really necessary... so I ended up with this:

type OnlyAllowThemeValueForKey<
  Key extends StyleableSxProperties,
  Scale extends
    | keyof DripsyThemeWithoutIgnoredKeys
    | StyleableSxProperties = Key extends keyof Scales
    ? // if Key = 'color'
      // then Scales['color'] = 'colors'
      // so if 'colors' is a keyof the theme
      // then the scale is indeed 'colors', and we tokenize theme.colors
      Scales[Key] extends keyof DripsyThemeWithoutIgnoredKeys
      ? Scales[Key]
      : Key
    : Key,
  AliasedScale = AliasToScale<Scale>,
  IsScaleInTheme extends boolean = AliasedScale extends keyof DripsyThemeWithoutIgnoredKeys
    ? true
    : false
> = IsScaleInTheme
nandorojo commented 1 year ago

Hey guys, I got this fixed on Dripsy v4. Thanks for your patience. It's tested on Typescript v5. You can see the upgrade guide here if you'd like to test the prerelease version: https://github.com/nandorojo/dripsy/releases/tag/v4.0.0

I've integrated it into BeatGig in prod successfully in our large monorepo.

I'm using dripsy-4.0.0-alpha.43 at the time of writing.