enhance types of mergeProps

[bigmistqke]

Describe the bug

mergeProps is a powerful tool, but the typing is a bit clunky.

Your Example Website or App

https://playground.solidjs.com/anonymous/f901c3ec-82cb-4fbf-bc81-6e6583c67b64

Steps to Reproduce the Bug or Issue

type Vector = [number, number];
type Props = { tuple: Vector };

const App = (props: Partial<Props>) => {
  const merged = mergeProps({ tuple: [1, 1] }, props);
  merged.tuple; // type is (number[] | Vector) & (number[] | Vector | undefined)
  // ideally types would not become less narrow by using mergeProps

  // typecasting works, but is clunky and not typesafe
  const merged2 = mergeProps({ tuple: [1, 1] as Vector }, props);
  merged2.tuple; // type is Vector
  const merged3 = mergeProps({ tuple: [1, 1] as Vector }, props) as Props;
  merged3.tuple; // type is Vector

  // no type-error
  const merged4 = mergeProps({ tuple: [1, 1, 1] }, props)
  // no type-error
  const merged5 = mergeProps({ tuple: [1, 1, 1] }, props) as Props

  return <></>;
};

Expected behavior

1. ideally types would not become less narrow by using mergeProps and no additional typecasts would be needed
2. original typing of props should be respected when merging props, or there is at least a typesafe way to enforce this.

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• Browser: [e.g. Chrome, Safari, Firefox]
• Version: [e.g. 91.1]

Additional context

No response

[otonashixav]

1. I mean this isn't really something fixable, typescript is going to infer [1, 1] as number[] and not [number, number], and I don't see any way to correctly type this without [1, 1] as const or [1, 1] satisfies Vector.
2. mergeProps doesn't know which of the arguments passed is the original type, and it doesn't make assumptions about what the user wants. You could make a wrapper that is meant to merge props onto default props and have the latter be the "original" type.

[ryansolid]

I imagine this is why the types for Object.assign are equally unhelpful. But yeah not really anything to do here as far as I understand.

[bigmistqke]

2. I was thinking something in the line of mergeProps taking a generic that is the 'correct' type and the result of mergeProps would be typechecked against this type. Then mergeProps<{tuple: Tuple}>({ tuple: [1, 1, 1] }, props) would error and you could do something like (props: Partial<Props>) => {const merged = mergeProps<Props>({...assign all values...}, props) ...}

[otonashixav]

Right now the generic is the entire list of args, which is necessary to preserve their order so that we can make sure that the args that come later override those which come earlier. So you can currently use mergeProps<{ tuple: Tuple }[]>. Though I guess you're expecting the output to be { tuple: Tuple }, and not all the inputs.

It should be possible to add an overload which takes a non-array generic to catch this usage, after the current one, if necessary. The problem I see with this is that the error message is going to be very vague if you're missing something (like, args is not assignable to never level of vague). On second thought this isn't possible if it is meant to type the output, since the input would still need to capture the actual types to check if they produce the correct output, and partial inference doesn't exist in typescript.

[xdev1]

Not really 100% an answer to the question, but I personally have always asked myself (I'm not an actual Solid user yet) why this general mergeProps function is always used explicitly for the "default props use case" in Solid (see Solid Tutorial "Props/Default Props"), instead of using a special additional function tailored for exact that "default props use case" and nothing else. I think the above snippet is such a "default props use case". It may have some advantages if the arguments were swapped, means f(props, defaultProps) instead of f(defaultProps, props), as defaultProps may be a large object literal, and so this argument order may be better to read and will not produce additional lines with prettier - maybe just a matter of taste 😉
Okay, long story short: Let's try to define a function withDefaults(props, defaultProps) that covers only this "default props use case", hoping proper typing becomes simpler then (like said, I know, that was not exactly the question, but anyway).
My little brain did not make it to find a proper type for this withDefaults function with arity 2, I always had the same problems that have already described in the other comments above.
But if you implement this withDefaults function in curried form, typing seems to become easier: Demo

Maybe currying could also help with typing a method mergeProps2(a)(b)(c)(), but I have not tried that ... and I guess most folks won't like such a mergeProps2 function anyway 😄 [Edit] Maybe mergeProps2(a)(b), mergeProps3(a)(b)(c), mergeProps4(a)(b)(c)(d) will be easier ...

[bigmistqke]

Okay, long story short: Let's try to define a function withDefaults(props, defaultProps) that covers only this "default props use case", hoping proper typing becomes simpler then (like said, I know, that was not exactly the question, but anyway).

Not exactly the question, but exactly the intent behind my question: a typesafe way of defining default props 👍

[otonashixav]

// typecheck defaultProps as Props by assigning it
const defaultProps: Props = { tuple: [1, 1] };
const mergedProps = mergeProps(defaultProps, props);
// or in TS 4.9 and later
const mergedProps = mergeProps({ tuple: [1, 1] } satisfies Props, props);

[bigmistqke]

This does give us typesafety while assigning, but the types of the result-value do not accurately portray this:

type Vector = [number, number];
type Color = string | { r: number; g: number; b: number };
type Props = { tuple: Vector; color: Color };

const App = (props: Partial<Props>) => {
  const defaultProps: Props = { tuple: [1, 1], color: "red" };
  const merged1 = mergeProps(defaultProps, props);
  // type merged1.color = Color & (Color | undefined);
  const merged2 = withDefaults(props)(defaultProps);
  // type merged2.color = Color & (Color | undefined);
  const merged3 = { ...props, ...defaultProps };
  // type merged3.color = Color;
  return <></>;
};

playground
[EDIT: fixed typo in return type of the withDefaults]

I could cast of course

  const defaultProps: Props = { tuple: [1, 1], color: "red" };
  const merged1 = mergeProps(defaultProps, props) as Props;

But it's a bit clunky imo. I would expect solid to provide me a more ergonomic way of doing this.

[xdev1]

It's a bit of a problem (at least it's confusing) that the base types are defined like this:

type Props = { tuple: Vector };

const App = (props: Partial<Props>) => {
  ...
}

instead of defining them in the usual way:

type Props = { tuple?: Vector };

const App = (props: Props) => {
  ...
}

Because then it will turn out that using the following pattern

  const defaultProps: Props = { tuple: [1, 1] };

would not be very useful any longer, but using the following will always work:

  const defaultProps = { tuple: [1, 1] } satisfies Props;

Would really help if Solid Playground was using a TS version >= 4.9 😉

Also @bigmistqke, in your latest demo you have changed the return type of the withDefaults function: It's => P & D, not just => P.

but the types of the result-value do not accurately portray this

Sorry, I do not understand ... which type in the demo is not fine?

[bigmistqke]

Edited the typo 👍

With satisfies and not using Partial-typehelper, withDefaults seems to be working.
+1 on Solid Playground using TS version >= 4.9

It's a bit of a problem (at least it's confusing) that the base types are defined like this:

I think it's pretty clean tbh. Sad that it has implications on the typings.

[ryansolid]

Is there any action to take here?

[bigmistqke]

Mm, well, withDefaults works as long as you do not use Partial-typehelper. I use Partial a lot, especially in typing props, but maybe that's just me. I am also not sure if withDefaults fits inside the rest of solid's API because of the currying.

I generally do not mind the lack of props-destructuring in solidjs, but for default props it is handy. At the moment there is not really a proper way to provide default props, mergeProps is being communicated in the docs as the best way to do it, but it has some DX/typing-issues as mentioned above. withDefaults is a step in the right direction.

[thetarnav]

You can contribute it to solid-primitives if you think it's useful 😉

[bigmistqke]

true, but I think in this case it would be good to have an 'official' solution that can be recommended in docs, since it's such a common task.

[ryansolid]

Ok, so basically TS can't do what we want, there is a solution with a different sort of API that uses currying which may be weird. I think the idea is interesting, but I'm also slow on adding new APIs to do something we already do a different way. Let me add this to my considerations for 2.0 list and in the meanwhile submit to solid-primitives to see how people like using it in practice.