johanholmerin / style9

CSS-in-JS compiler inspired by Meta's stylex
MIT License
570 stars 27 forks source link

Fix style9 params type to allow falsy values #45

Closed TxHawks closed 3 years ago

TxHawks commented 3 years ago

Typing style9's params as Style[] does not account for it being able to also take falsy values - and ignore theme - which is helpful for style composition.

There is even already a test for this in tests/resolver.js#L28-L35, so this really only aligns the type information with existing functionality.

johanholmerin commented 3 years ago

Also needs a test, could add it to the end of types/test/basic.ts

TxHawks commented 3 years ago

Re the numbers and empty strings, no problem, though it does represent the actual way the code works and might be confusing. Since JS type coercion is used in the code, why not represent it in the type information as well? I will, Of course, defer to your decision.

I didn't include tests since one that covers this case already exists in tests/resolver.js#L28-L35. But you are right that it covers style9 and not style returned from style9.create. I'll add these now.

Also, I just realized that the typing of the signature of the function returned from style9.create isn't technically correct, since

((
  ...names: Array<
    keyof T | Falsy | { [key in keyof T]?: boolean | undefined | null }
  >
) => string);

Would suggest that style(false) would work, while it actually throws Unsupported type BooleanLiteral, and only style(false && 'name' ) works. But I'm not sure how to type that in typescript.

johanholmerin commented 3 years ago

I would prefer to keep the types strict. We can always expand them in the future.

The function returned from style9.create only exists as a type definition. The compiler removes the calls and replaces it with the class string. So there is not implementation test to update. What I linked to was the tests for the type definition, which is what you are updating.

style(false) is not supported because it's not a meaningful use. There is no way to type that, so you can ignore that condition.

TxHawks commented 3 years ago

Updated