solidjs-community / solid-primitives

A library of high-quality primitives that extend SolidJS reactivity.
https://primitives.solidjs.community
MIT License
1.23k stars 122 forks source link

combineProps type error since typescript 5.3 #554

Open GiyoMoon opened 9 months ago

GiyoMoon commented 9 months ago

Describe the bug

I noticed that combineProps throws a type error when upgrading to typescript 5.3. In typescript <=5.2, the same code worked without issues.

import { combineProps } from '@solid-primitives/props'
import { Dynamic, DynamicProps } from 'solid-js/web'
import type { ValidComponent } from 'solid-js'

const MyComponent = <T extends ValidComponent = 'div'>(
  props: DynamicProps<T>,
) => {
  const combinedProps = combineProps([props])
  const children = <div>children</div>
  // This worked with typescript version <= 5.2 and broke in 5.3
  return <Dynamic {...combinedProps}>{children}</Dynamic>
}

export default MyComponent
image

Minimal Reproduction Link

https://github.com/GiyoMoon/solid-primitives-type-error

thetarnav commented 5 days ago

Currently combineProps is just using the MergeProps type from solid, so mergeProps has the same issue from what I see: https://playground.solidjs.com/anonymous/06bd1b95-c609-4d66-93ef-656ee931fba2

Also it's interesting how the same code has no problems without jsx: image

I get the same error for compineProps, even in .ts file, if I slightly change the function signature:

// from
export function combineProps<T extends [] | MaybeAccessor<PropsInput>[]>(...sources: T): MergeProps<T>
// to
export function combineProps<T extends unknown[]>(...sources: T): MergeProps<T>

Which shouldn't change anything

Ok it has nothing to do with MaybeAccessor<PropsInput> part, the same difference is also between unknown[] and [] | unknown[], where the latter just makes the type to be inferred as a tuple.

thetarnav commented 5 days ago

Overwriting the children prop in combineProps rather than jsx seems to fix it. Which is probalby a better way in general, because it doesn't cause another mergeProps to be compiled in to handle the overwritting.

const merged = combineProps(props, {
  get children(){
    return <div>children</div>
  }
})
return <Dynamic {...merged}/>

Maybe this should be reopened on solidjs repo as well. Although the usage seems incorrect, because of mentioned issue with multiple mergeProps.