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

Typing mismatch for fontWeight property (maybe others?) #124

Closed
olivierpascal opened this issue Dec 10, 2023 · 11 comments · Fixed by #125
Closed

Typing mismatch for fontWeight property (maybe others?) #124

olivierpascal opened this issue Dec 10, 2023 · 11 comments · Fixed by #125

Comments

@olivierpascal
Copy link

olivierpascal commented Dec 10, 2023

The problem

fontWeight has to be a string as per TTokens, but cannot be a string as per StyleXCSSTypes.

How to reproduce

I want to do:

// tokens.stylex.ts

import * as stylex from '@stylexjs/stylex';

export const typo = stylex.defineVars({
  fontWeight: '400',
});

// fontWeight has to be a string, as per @stylexjs/stylex/lib/StyleXTypes.d.ts:177:
// type TTokens = Readonly<{
//   [key: string]: string | { [key: string]: string };
// }>;
// MyComponent.tsx

import type { StyleXStyles } from '@stylexjs/stylex';
import * as stylex from '@stylexjs/stylex';

type Styles = {
  container: StyleXStyles;
};

const styles = stylex.create({
  container: {
    fontWeight: typo.fontWeight,
  },
});

// ...

BUT :

// @stylexjs/stylex/lib/StyleXCSSTypes.d.ts

// this cannot be an arbitrary string like '400'
type fontWeight =
  | 'inherit'
  | 'normal'
  | 'bold'
  | 'bolder'
  | 'lighter'
  | 100
  | 200
  | 300
  | 400
  | 500
  | 600
  | 700
  | 800
  | 900;

So I got:

Type 'StyleXClassNameFor<"fontWeight", string>' is not assignable to type 'StyleXClassNameFor<"fontWeight", Readonly<"initial" | "inherit" | "unset" | "normal" | "bold" | "bolder" | "lighter" | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900 | null | undefined>>

@olivierpascal olivierpascal changed the title string only type for variable properties is too restrictive Typing mismatch for fontWeight property (maybe others?) Dec 10, 2023
@nmn
Copy link
Contributor

nmn commented Dec 10, 2023

This is a bug in our type definition. Your usage is correct. Will fix.

@purohitdheeraj
Copy link
Contributor

purohitdheeraj commented Dec 11, 2023

@nmn can i take this please,
we only need replacement of that right ? from integer to string
or can add additional string type

@kevintyj
Copy link
Contributor

type fontWeight should also support of type number (variable fonts) but I feel that for tokens, it should accept number values (other than just strings) and then from the create function reject using StyleXCSSTypes
ie: making a token for padding and spacing values as numbers

@nmn
Copy link
Contributor

nmn commented Dec 11, 2023

@kevintyj There are some types functions that I'm working on that will make some of this better. We don't accept number values for tokens yet because while StyleX will usually convert numbers to px values based on the property name, variables don't have a type.

In the meantime we have two options:

  1. Add string equivalents for all the number values: '100' | '200' | .... This is safe and would compile to exact same thing.
  2. Simply add string | number at the end of the list of types. This would allow arbitrary number values that are not multiples of 100.

Considering the increasing use of variable fonts, I think option 2 is good for this.

@purohitdheeraj You are welcome to put up this PR, but be prepared for some discussion about the better option mentioned above.

@kevintyj
Copy link
Contributor

@nmn

Considering the increasing use of variable fonts, I think option 2 is good for this.

I agree that option 2 would be a better temporary fix until the new type function is adopted! The only downside being that arbitrary number would not satisfy type def of TTokens as it only accepts string

@timwehrle
Copy link
Contributor

timwehrle commented Dec 11, 2023

@kevintyj You‘re right but I guess this approach in the following code could work. And if I understand this right, this allows strings “400”, “bolder” and numeric values like 400? But I guess it’s a bit more complicated than that. 😄

// /packages/stylex/src/StyleXTypes.js

type TTokens = $ReadOnly<{
  [string]:
    | number
    | string
    | $ReadOnly<{ default: number | string, [string]: number | string }>,
}>;

@nmn
Copy link
Contributor

nmn commented Dec 11, 2023

@timwehrle The nested object case is handled elsewhere. The fix would be as simple as:

type fontWeight =
  | 'inherit'
  | 'normal'
  | 'bold'
  | 'bolder'
  | 'lighter'
  | 100
  | 200
  | 300
  | 400
  | 500
  | 600
  | 700
  | 800
  | 900
  | string
  | number;

@purohitdheeraj The race is on!


arbitrary number would not satisfy type def of TTokens as it only accepts string

Yeah, as explained we cannot accept raw numbers as variable values. It is error prone because you may expect some number to be auto-converted to px values which won't happen.

The types function will let you do something like this, which should solve it:

const vars = stylex.defineVars({
  heavy: stylex.types.number(700),
  red500: stylex.types.color('tomato'),
  size_lg: stylex.types.length('64px'),


  // And I'm thinking about the following patterns:
  size_sm: stylex.types.rem(2),
  size_md: stylex.types.px(36),

  // Further, I want something like this, but it needs to be immutable/unthemeable:
  media_mobile: stylex.types.media('(max-width: 480px)'),
});

I'll write a full proposal with details about the types function, but know that it will end up generating @property --varname declarations which will give your variables types even in CSS. This can be useful for a bunch of things including being able to animate CSS variable values.

I'm also working on a full CSS parser which will be used to validate the values being passed to the type function.

@purohitdheeraj
Copy link
Contributor

purohitdheeraj commented Dec 11, 2023

hello everyone, this question might be silly

i made the changes in StyleXCSSTypes.d.ts fontWeight type
but its not getting saved and not identified by git
its ignored by gitlens

what should be the next step

  • do i need to run test or build , its package file

even if someone has already solved it
please guide me how make changes in such package files

@kevintyj
Copy link
Contributor

kevintyj commented Dec 11, 2023

StyleXCSSTypes.d.ts

It should work? I just tested it out and was able to make a commit to that file. Perhaps check your local gitignore config?

git config --get core.excludesfile

EDIT I also dont think thats the file you should edit, it should be the StyleXCSSTypes.js file

@nmn
Copy link
Contributor

nmn commented Dec 11, 2023

@purohitdheeraj Please edit the .js file in src/. StyleX is authored in Flow and the TS types are generated from that. You're looking for StyleXCSSTypes.js.

After that, running npm run build will update the auto-generated StyleXCSSTypes.d.ts with the new type.

NOTE: StyleXTypes.d.ts (not StyleXCSSTypes.d.ts) is the only typescript file in the Git repo. It is needed for workarounds for types that can't be converted from Flow to Typescript. Everything else is generated.

@purohitdheeraj
Copy link
Contributor

Thank you so much @nmn and @kevintyj
just raised a PR for it

@nmn nmn closed this as completed in #125 Dec 11, 2023
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 a pull request may close this issue.

5 participants