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

Add type for PropsWithRef to define components that accept refs #778

Closed
nksaraf opened this issue Dec 31, 2021 · 14 comments · Fixed by #877
Closed

Add type for PropsWithRef to define components that accept refs #778

nksaraf opened this issue Dec 31, 2021 · 14 comments · Fixed by #877
Labels
typescript relating to typescript or types
Milestone

Comments

@nksaraf
Copy link
Member

nksaraf commented Dec 31, 2021

This type will be helpful and will the serve the purpose that forwardRef in react did. It allows us to easily define the ref type. The ref prop should be a function should that people don't use it by mistake and know that they either have to pass it or call it with the reference.

eg.

type PropsWithRef<T, Props> = Props & { ref: (val: T) => void 

function Box(props: PropsWithRef<HTML.DivElement, { color: string }) {
	return <div ref={props.ref} />
}
@lxsmnsyc
Copy link
Member

How does that work for:

let ref;

<Box ref={ref} />

Take note that Solid supports this.

@nksaraf
Copy link
Member Author

nksaraf commented Dec 31, 2021

Yeah I know this works.. but when the ref comes from a parent.. its typed as a function, and annoying to type ourselves.. and more like an implementation detail

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Dec 31, 2021

Not exactly. Even though it is typed as a function after compilation, we lose support to the feature I mentioned which is like the most common way to capture refs. We should be consistent. In solid-headless, a union type for ref works:

type Ref<T> = T | (val: T) => void;

But normally, you only have to do this in the component:

if (typeof props.ref === 'function') {
  props.ref(el);
}

of course, this is seamless:

<div ref={props.ref} />

@nksaraf
Copy link
Member Author

nksaraf commented Dec 31, 2021

Ohh I thought passing a function in to ref is acceptable.. is that typed as an error? And is this Ref type exported from solid-js?

@lxsmnsyc
Copy link
Member

passing a ref as a function is acceptable. My point was doing let ref with your proposed type doesn't work and so users are forced to write a ref callback, even though the Solid compiler supports the expression.

@nksaraf
Copy link
Member Author

nksaraf commented Dec 31, 2021

I don't thing the type should be used with let ref.. its only for props with ref that needs to be forwarded.. that has the awkward type.. so

let ref: Ref<T>;

but

type PropsWithRef<T, Props> = Props & { ref: T | (val: T) => void }

function Box(props: PropsWithRef<HTML.DivElement, { color: string }) {
	return <div ref={props.ref} />
}

ohh yeah maybe ur right in that those props need to include T | (val: T) => void

but I adding that makes sure that the user checks the function before trying to assign to it or something

@lxsmnsyc
Copy link
Member

Surely, that's the downside, however usage of the ref feels more natural.

@nksaraf
Copy link
Member Author

nksaraf commented Dec 31, 2021

Yeah no I agree being more explicit is better about the ref, but do we have the base Ref type in Solid?

@lxsmnsyc
Copy link
Member

We don't. It's just an impromptu type I wrote 😂

@ryansolid ryansolid added the typescript relating to typescript or types label Dec 31, 2021
@trusktr
Copy link
Contributor

trusktr commented Feb 6, 2022

The problem here is that we're assigning to someRef in ref={someRef} (f.e. <div ref={someRef}> assigns an HTMLDivElement instance (at runtime) to someRef, but TypeScript considers the operation (type-wise) to be the opposite: TS sees the type of the ref={someRef} expression as assigning someRef to the ref prop.

I'm not sure how to improve this, because we can't make TypeScript think in the other direction here.

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Feb 7, 2022

@trusktr wouldn't really matter since we'll use it extend our props from anyway

@atk
Copy link
Contributor

atk commented Feb 7, 2022

I fail to see how this has added value over Component<{ ref: … }>?

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Feb 7, 2022

@atk I rarely use Component<T> because you can't really use generics on it. Also, Component<T> has children pre-defined.

declare function SomeComp<T>(props: SomeProps<T>): JSX.Element

@atk
Copy link
Contributor

atk commented Feb 7, 2022

Thanks for the clarification.

I think we should make the component/props types composable. There was also a discussion to add VoidComponents for components that do not allow children (because those would never be rendered).

If we touch the types to define components, we should aim for a more complete version.

edemaine added a commit to edemaine/solid that referenced this issue Mar 7, 2022
* Rename past `Component` type to `ComponentWithChildren`.
  Motivation: Most components don't actually support children.
  This causes a type error upon accidental passing of children /
  forces you to think about which components support children.
* Added second parameter to `ComponentWithChildren`
  to make it easier to give a custom type for `children`
  (but still defaults to useful `JSX.Element`).
* New `Component` type does not have automatic `children` property
  (like React's preferred `VoidFunctionalComponent`),
  offering another natural way to type `props.children`:
  `Component<{children: JSX.Element}>`.
* `props` argument in both `Component` and `ComponentWithChildren`
  automatically cast to `readonly` (shallow one level only),
  to avoid accidental assignment to `props.foo` (usually a getter)
  while still allowing passing mutables in props.
  Add `Props<T>` helper for this transformation.
* Add @lxsmnsyc's `Ref<T>` type so it's easy to type `props.ref`:
  `Component<{ref: Ref<Element>}>`.
  <solidjs#778 (comment)>
edemaine added a commit to edemaine/solid that referenced this issue Mar 7, 2022
* Rename past `Component` type to `ComponentWithChildren`.
  Motivation: Most components don't actually support children.
  This causes a type error upon accidental passing of children /
  forces you to think about which components support children.
* Added second parameter to `ComponentWithChildren`
  to make it easier to give a custom type for `children`
  (but still defaults to useful `JSX.Element`).
* New `Component` type does not have automatic `children` property
  (like React's preferred `VoidFunctionalComponent`),
  offering another natural way to type `props.children`:
  `Component<{children: JSX.Element}>`.
* `props` argument in both `Component` and `ComponentWithChildren`
  automatically cast to `readonly` (shallow one level only),
  to avoid accidental assignment to `props.foo` (usually a getter)
  while still allowing passing mutables in props.
  Add `Props<T>` helper for this transformation.
* Add @lxsmnsyc's `Ref<T>` type so it's easy to type `props.ref`:
  `Component<{ref: Ref<Element>}>`.
  <solidjs#778 (comment)>
  Fixes solidjs#778.
ryansolid added a commit that referenced this issue May 3, 2022
* Revised Component types (children, readonly, ref)

* Rename past `Component` type to `ComponentWithChildren`.
  Motivation: Most components don't actually support children.
  This causes a type error upon accidental passing of children /
  forces you to think about which components support children.
* Added second parameter to `ComponentWithChildren`
  to make it easier to give a custom type for `children`
  (but still defaults to useful `JSX.Element`).
* New `Component` type does not have automatic `children` property
  (like React's preferred `VoidFunctionalComponent`),
  offering another natural way to type `props.children`:
  `Component<{children: JSX.Element}>`.
* `props` argument in both `Component` and `ComponentWithChildren`
  automatically cast to `readonly` (shallow one level only),
  to avoid accidental assignment to `props.foo` (usually a getter)
  while still allowing passing mutables in props.
  Add `Props<T>` helper for this transformation.
* Add @lxsmnsyc's `Ref<T>` type so it's easy to type `props.ref`:
  `Component<{ref: Ref<Element>}>`.
  <#778 (comment)>
  Fixes #778.

* Fix test

* Remove Props helper and shallow Readonly for props

* VoidComponent, ParentComponent, FlowComponent

* Use Component<any> for generic component type

* Add default types, Context.Provider require children

Comments from Otonashi

* Default parameter for PropsWithChildren

* Restore missing <any>

* Docs typo fix

* Cleanup server code

* JSDoc improvements

* Improve FlowProps JSDoc

* More FlowComponent JSDoc improvements

Co-authored-by: Ryan Carniato <ryansolid@gmail.com>
@ryansolid ryansolid added this to the 1.4.0 milestone May 9, 2022
daivd-patrick added a commit to daivd-patrick/Solid that referenced this issue Jun 10, 2023
* resource and reactive updates, fix #919

* stores: top level arrays, prevent obj overnotify, fixes #960 batching mutable

* fix #940 undefined ssr, fix #955 undefined className, fix #958 undefined spread

* changelog wip

* fix: fix the signal setter type when used with generics (#910)

This makes it easier to create generic wrappers around `createSignal` e.g.
```ts
function createGenericSignal<T>(): Signal<T | undefined> {
  const [generic, setGeneric] = createSignal<T>();
  const customSet: Setter<T | undefined> = (v?) => setGeneric(v);
  return [generic, (v?) => setGeneric(v)];
}

function createInitializedSignal<T>(init: T): Signal<T> {
  const [generic, setGeneric] = createSignal<T>(init);
  const customSet: Setter<T> = (v?) => setGeneric(v);
  return [generic, (v?) => setGeneric(v)];
}
```
where previously such the implementation would not be assignable to `Setter`, and would need to be cast.

Unexpectedly this also makes this more specific:
```ts
const [s, set] = createSignal<string>(); // <-- signal contains undefined
// before:
const v = set(() => "literal");
//    ^? - string
// after:
const v = set(() => "literal");
//    ^? - "literal"
```

* refactor: improve splitProps type for arbitrary rest args length (#930)

- Allows the return type of splitProps to be correct for 1 or more `...keys` passed
- Removes overloads for `splitProps` as they are no longer necessary
- Types each array passed as rest args as `readonly` as they do not need to be mutable

* v1.4.0-beta.0

* fix: spreading non-object props (#963)

- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes #958

* Revised Component types (children, ref) (#877)

* Revised Component types (children, readonly, ref)

* Rename past `Component` type to `ComponentWithChildren`.
  Motivation: Most components don't actually support children.
  This causes a type error upon accidental passing of children /
  forces you to think about which components support children.
* Added second parameter to `ComponentWithChildren`
  to make it easier to give a custom type for `children`
  (but still defaults to useful `JSX.Element`).
* New `Component` type does not have automatic `children` property
  (like React's preferred `VoidFunctionalComponent`),
  offering another natural way to type `props.children`:
  `Component<{children: JSX.Element}>`.
* `props` argument in both `Component` and `ComponentWithChildren`
  automatically cast to `readonly` (shallow one level only),
  to avoid accidental assignment to `props.foo` (usually a getter)
  while still allowing passing mutables in props.
  Add `Props<T>` helper for this transformation.
* Add @lxsmnsyc's `Ref<T>` type so it's easy to type `props.ref`:
  `Component<{ref: Ref<Element>}>`.
  <solidjs/solid#778 (comment)>
  Fixes #778.

* Fix test

* Remove Props helper and shallow Readonly for props

* VoidComponent, ParentComponent, FlowComponent

* Use Component<any> for generic component type

* Add default types, Context.Provider require children

Comments from Otonashi

* Default parameter for PropsWithChildren

* Restore missing <any>

* Docs typo fix

* Cleanup server code

* JSDoc improvements

* Improve FlowProps JSDoc

* More FlowComponent JSDoc improvements

Co-authored-by: Ryan Carniato <ryansolid@gmail.com>

* refactor: simplify effect types (#913)

- Simplified `createEffect` etc. such that they no longer use rest args
- Added test cases for effect functions failing partial generic inference (not supported by typescript)
- Removed extra params from effects' first overload

* optimize: use jest fake timer to test suspense (#964)

fix #761

* new store modifiers splice, modifyMutable, bug fixes

* v1.4.0-beta.1

* refactor: remove extra generic from modifyMutable (#965)

* fix splice length setting

* remove unnecessary splice

* feat: resource deferred streaming

* v1.4.0-beta.2

* support standard JSX transform

* fix #967 `onHydrated`

* v1.4.0-beta.3

* Different types for standard transform

* v1.4.0-beta.4

* fix solid hyperscript, startTransition cleanup

* fix build

* v1.4.0-beta.5

* fix synchronous resources

* more changelog updates

* fix batch consistency in stores

* v1.4.0-beta.6

* typescript: correct on helper types (#959)

* refactor: correct on helper types

- Previous input type now includes undefined, which is its initial value
- When defer is true, memos created with the on helper include undefined
- Fixed return type becoming unknown when passing a function with a third parameter (the previous type)
- Allow readonly arrays/tuples to be passed as deps
- Disallow empty arrays from being passed as deps
- Breaks backwards compatibility of the first generic; now it is the type of the inputs passed to the function instead of the type of the deps themselves:
```ts
// before
on<number>(() => 1, ...) // error
on<() => number>(() => 1, ...) // ok
on<[number]>([() => 1], ...) // error
on<[() => number]>([() => 1], ...) // ok

// now
on<number>(() => 1, ...) // ok
on<() => number>(() => 1, ...) // error
on<() => number>(() => () => 1, ...) // ok
on<[number]>([() => 1], ...) // ok
on<[() => number]>([() => 1], ...) // error
on<[() => number]>([() => () => 1], ...) // ok
```

* docs: update on helper jsdoc

* fix: missing readonly in on helper types' `AccessorTuple`

* refactor: allow any array in `AccessorTuple`

- So that passing arrays works
- Renamed to `AccessorArray`

Co-authored-by: Ryan Carniato <ryansolid@gmail.com>

Co-authored-by: Xavier Loh <42372774+otonashixav@users.noreply.github.com>
Co-authored-by: Erik Demaine <edemaine@mit.edu>
Co-authored-by: WZT <genuifx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript relating to typescript or types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants