Skip to content

types: changing UnwrapRef to a simpler implementation #579

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

Merged
merged 10 commits into from
Apr 15, 2020

Conversation

pikax
Copy link
Member

@pikax pikax commented Jan 4, 2020

This is a type I was really careful not to break anything, I hope I haven't missed anything.

Overall this shouldn't affect much, but it improves when using Ref on a generic function.

I'm porting my vue composables library to use the new reactivity and because of the nature of the library, my functions are quite generic, and ran into some issues with the typescript type checking.

It works using @vue/vue-composition, but when I use @vue/reactivity it has some errors, and not really readable errors since the typescript type is quite weird to because of the recursive implementation.

Full method working with plugin

Reproduction code

const f = <T extends Promise<any>, TArgs extends Array<any>>(
  fn: (...args: TArgs) => T
) => {
  const t: Ref<T> = {} as any
  const a: TArgs = {} as any
  t.value = fn(...a) // doesn't work, because return value is not Ref<T>
  t.value = fn(...a) as UnwrapRef<T> // doesn't work with prev UnwrapRef but works with this PR
}

I don't believe with a recursive approach we will be able to fix this without a cast, because I've raised microsoft/TypeScript#33942 some time ago, but seems to be a design limitation so I don't expect it to change any time soon.

EDIT: Having issues with Portal and inferring the children on h

EDIT2: Still having issues with the portal seems to not be inferred, the issue being with ref<VNodeChildren<TestNode, TestElement>> be able to be converted to RawChildren therefor not be able to match with the overload

Argument of type 'UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null>' is not assignable to parameter of type 'RawChildren'.
  Type 'UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null>' is not assignable to type 'VNodeChildren<any, any>'.
    The types returned by 'pop()' are incompatible between these types.
      Type 'string | number | boolean | void | UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null> | UnwrappedObject<...> | null | undefined' is not assignable to type 'string | number | boolean | void | VNodeChildren<any, any> | VNode<any, any> | null | undefined'.
        Type 'UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null>' is not assignable to type 'string | number | boolean | void | VNodeChildren<any, any> | VNode<any, any> | null | undefined'.
          Type 'UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null>' is not assignable to type 'VNodeChildren<any, any>'.
            The types returned by 'pop()' are incompatible between these types.
              Type 'string | number | boolean | void | UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null> | UnwrappedObject<...> | null | undefined' is not assignable to type 'string | number | boolean | void | VNodeChildren<any, any> | VNode<any, any> | null | undefined'.
                Type 'UnwrappedArray<string | number | boolean | void | VNodeChildren<TestNode, TestElement> | VNode<TestNode, TestElement> | null>' is not assignable to type 'string | number | boolean | void | VNodeChildren<any, any> | VNode<any, any> | null | undefined'.

EDIT3: It can be fixed by casting to the correct type:

h(Portal, { target }, children.value as VNodeChildren<TestNode, TestElement>)

EDIT4: merged with master and made a few changes to be able to pass the testing.
Has been some time since I opened this PR, but my problem was getting error:

t.value = fn(...a) // it works in the PR and master!? 😕 
t.value = fn(...a) as UnwrapRef<T> // doesn't work PR and master 😕 

The value of this PR is the error messages, instead of having the unreadable message from recursion, you have a message like:

Type 'UnwrapRef<T>' is not assignable to type 'T'.
  Type 'unknown' is not assignable to type 'T'.
    Type 'UnwrappedObject<Promise<any>>' is not assignable to type 'T'.ts(2322)

Probably in the next few days I will do a bit more testing

@pikax pikax changed the title types: changing UnwrapRef to a simpler implementation [WIP] types: changing UnwrapRef to a simpler implementation Jan 4, 2020
pikax and others added 4 commits January 4, 2020 14:09
# Conflicts:
#	packages/reactivity/src/ref.ts
#	packages/runtime-core/__tests__/apiTemplateRef.spec.ts
#	packages/runtime-core/src/apiWatch.ts
@pikax pikax changed the title [WIP] types: changing UnwrapRef to a simpler implementation types: changing UnwrapRef to a simpler implementation Apr 8, 2020
@pikax
Copy link
Member Author

pikax commented Apr 10, 2020

@yyx990803 is this useful or just close it?

@yyx990803
Copy link
Member

Can you remove the formatting changes from this PR?

@pikax
Copy link
Member Author

pikax commented Apr 15, 2020

formatting fixed after merging 1068212

| CollectionTypes
| BaseTypes
| Ref
| Element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary since BaseTypes includes Node

? Tupple<T> extends never ? Array<V> : UnwrapTupple<T>
: T extends object ? UnwrappedObject<T> : T

export type UnwrapTupple<T> = { [P in keyof T]: T[P] } & {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type UnwrapTupple<T> = { [P in keyof T]: T[P] } & {
export type UnwrapTuple<T> = { [P in keyof T]: T[P] } & {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not fully understanding what case UnwrapTuple is supposed to handle - can you add a type test case for it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think is needed, since we don't unwrap arrays anymore, was to fix the type inferring of

type UnwrapArray<T> = { [P in keyof T]: UnwrapRef<T[P]> }
// the array would be converted
/*
[1, 2, '1']
to 
{
 '1': 1,
 '2': 2,
 '3: '1'
}
*/

@yyx990803 yyx990803 merged commit e33291b into vuejs:master Apr 15, 2020
@yyx990803
Copy link
Member

Oops, looks like I hit the button at the wrong time

@yyx990803
Copy link
Member

Sorry for the accidental merge, can you open a follow up PR for the type test fix please?

@pikax
Copy link
Member Author

pikax commented Apr 15, 2020

Sorry about that, should have run the testing before pushing

@pikax pikax deleted the changing_unwrap_ref branch August 15, 2021 09:12
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