Skip to content
This repository has been archived by the owner on Apr 29, 2023. It is now read-only.

WIP: Only way found to fix nasty erros on TS build. #92

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

Conversation

tachyon-ops
Copy link

  • Reason for export function Link(props: LinkProps): VNode<LinkProps> | any; change request:
    TS2605: JSX element type 'void | VNode<LinkProps>' is not a constructor function for JSX elements.
  Type 'void' is not assignable to type 'Element | null'.
  • Reason for export function Route<P>( props: RouteProps<P>): VNode<RenderProps<P>> | any; change request:
    TS2605: JSX element type 'VNode<RenderProps<{}>>' is not a constructor function for JSX elements.
  Type 'VNode<RenderProps<{}>>' is missing the following properties from type 'Element': type, props

Let me know if you guys find exactly which type we should declare.

@codecov
Copy link

codecov bot commented Dec 2, 2018

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #92   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines          68     68           
  Branches       11     11           
=====================================
  Hits           68     68

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97b0a07...c59997c. Read the comment docs.

@tachyon-ops tachyon-ops changed the title Only way found to fix nasty erros on TS build. WIP: Only way found to fix nasty erros on TS build. Dec 2, 2018
@Swizz
Copy link
Contributor

Swizz commented Dec 3, 2018

The first warning seem explicitly enough for me. I would move on VNode<LinkProps> | null.
A VNode must not be undefined but can be null.
For the second, I guess Hyperapp.VNode should extends JSX.Element.

Plus, returns seem incorrect, as since #59 helpers do not return VNode, they return Lazy Components.

@tachyon-ops
Copy link
Author

@Swizz

declare global {
  namespace JSX {
    interface Element extends VNode<any> {}
    interface IntrinsicElements {
      [elemName: string]: any
    }
  }
}

Tested null for the first and it didn't work as well...
@Swizz have you tested your suggestions?

@Swizz
Copy link
Contributor

Swizz commented Dec 3, 2018

Untested. I knew that DT use JSX.Element | null on some places :
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/8ff7e8f29e059f3799c19e0bafb7ac010786ca50/types/react/v15/index.d.ts#L314

I think it is more related to Element and not to the null as any match with Element | null.

So I would try to extend Hyperapp.VNode from JSX.Element.

@tachyon-ops
Copy link
Author

But this means an update on the hyperapp dependency. What's your take on this @jorgebucaran ?

@jorgebucaran
Copy link
Owner

@nmpribeiro Maybe someone with more TS experience can comment.

@tachyon-ops
Copy link
Author

At least someone able to understand the return types of these JSX.Element and the VNode. h function should only expect JSX.Element right (at least only VNode)?. I will look at that again when possible.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants