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

types: improve type inference for cx concatenation #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iddar
Copy link

@iddar iddar commented Nov 20, 2024

Improved Type Inference for cx Function

This PR enhances the type system for the cx function to provide more accurate type literals in the return type without affecting runtime performance.

image

Changes

  • Added FilterStrings type helper to process tuple types
  • Added JoinStrings type helper to generate exact string literal types
  • Updated function signature to preserve string literals

Type System Improvements

Before

cx("foo", "bar")        // type: string
cx("foo", null, "bar")  // type: never

After

cx("foo", "bar")        // type: "foo bar"
cx("foo", null, "bar")  // type: "foo bar"
cx("a", "b", "c")       // type: "a b c"
// Conditional types are preserved
const test5 = cx("foo", false ? "bar" : "baz") satisfies "foo bar" | "foo baz";

Implementation

The solution uses TypeScript's type system to:

  1. Filter valid string arguments from the input tuple
  2. Join the filtered strings with spaces
  3. Preserve conditional types and unions
type FilterStrings<T extends readonly any[]> = T extends readonly [infer F, ...infer R]
  ? F extends string
    ? [F, ...FilterStrings<R>]
    : FilterStrings<R>
  : [];

type JoinStrings<T extends readonly string[]> = T extends readonly [infer F, ...infer R]
  ? F extends string
    ? R extends readonly string[]
      ? R['length'] extends 0
        ? F
        : `${F} ${JoinStrings<R>}`
      : F
    : never
  : '';

Justification

  • Improves DX by providing exact type information
  • Helps catch type errors at compile time
  • No runtime performance impact (types are erased during compilation)
  • Better IDE support with accurate autocompletion

Notes

  • This is a type-only change
  • No runtime behavior modifications
  • Fully backward compatible
  • No bundle size impact

cx(...["foo", "bar"]) Considered but not included

While analyzing type improvements, we found an edge case with spread arrays:

cx(...["foo", "bar"]) // Currently types as: `${string} ${string}`

when the array is statically known the workaround is to use the as const assertion:

cx(...["foo", "bar"] as const) // types as: "foo bar"

Same situation for object properties:

const obj = {  b: "bar" };
cx("foo, obj.b) // Currently types as: `${string} ${string}`
// fix
const obj = {  b: "bar" } as const;
cx("foo", obj.b) // types as: "foo bar"

We decided not to cover this case because:

  1. Type System Complexity: Adding support for spread arrays would significantly increase type system complexity.

  2. Common Usage: This pattern is rarely used in practice, as most calls to cx use direct string literals or variables.

Please let me know if you'd like me to explain any part of the implementation in more detail.

• Return exact string literal types instead of plain string
• Handle conditional types and unions correctly
• No runtime changes or performance impact

Before:
  cx("foo", "bar") → type: string
  cx("foo", null, "bar") → type: never

After:
  cx("foo", "bar") → type: "foo bar"
  cx("foo", null, "bar") → type: "foo bar"
@alexnault
Copy link
Owner

alexnault commented Nov 22, 2024

Hey @iddar, thanks for the PR! I really appreciate it.

While I think this is an interesting feature, I'm debating adding it for two reasons :

  • I'm not entirely sure about the value this output type brings and what errors it can catch. Do you have an example?
  • I worry about TS compile time and the exponential type it creates with conditionals (a more common scenario than cx("a", "b")), e.g.:
image

Also, your "Before" example shows:

cx("foo", null, "bar")  // type: never

I tried it and I'm getting:

cx("foo", null, "bar")  // type: string

Which is the expected behavior.


Again, I'm not against the idea, I'm just on the fence.
So let me know what you think!

Cheers!

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.

2 participants