jorgebucaran / superfine

Absolutely minimal view layer for building web interfaces
https://git.io/super
MIT License
1.57k stars 78 forks source link

fix h types #105

Closed dancespiele closed 6 years ago

dancespiele commented 6 years ago

with the type which there is now is not compatible with this:

{
  nodeName: "div",
  attributes: {},
  children: [
    {
      nodeName: "h1",
      attributes: {},
      children: 0
    },
    {
      nodeName: "button",
      attributes: { ... },
      children: "-"
    },
    {
      nodeName: "button",
      attributes: { ... },
      children: "+"
    }
  ]
}

to make it compatible is required to change the h types to:

export type Children = VNode | string | number | null;

export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes,
  children?: Array<Children> | Children,
): VNode<Attributes>

instead of

export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes,
  ...children: Array<VNode | string | number | null>
): VNode<Attributes>

because this ...children: Array<VNode | string | number | null> is translated to:

{
  nodeName: "div",
  attributes: {},
    {
      nodeName: "h1",
      attributes: {},
      children: 0
    },
    {
      nodeName: "button",
      attributes: { ... },
      children: "-"
    },
    {
      nodeName: "button",
      attributes: { ... },
      children: "+"
    }
}

therefore it is not possible to put an array after the attributes

codecov-io commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #105   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         124    124           
  Branches       40     40           
=====================================
  Hits          124    124

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 00d62ce...7123160. Read the comment docs.

jorgebucaran commented 6 years ago

@dancespiele Good, what about this usage? Is it covered too somehow in this PR?

h(nodeName, attributes, child1, child2, child3)
dancespiele commented 6 years ago

@jorgebucaran with this

export type Children = VNode | string | number | null;

export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes | null,
  children?: Array<Children> | Children
): VNode<Attributes>

allow does this: h(nodeName, attributes, child1, child2, child3) or: h(nodeName, attributes, [child1, child2, child3]) and more possibilities

see export type Children = VNode | string | number | null;

jorgebucaran commented 6 years ago

@dancespiele Okay, that's interesting. I don't know understand how h(nodeName, attributes, child1, child2, child3) can be supported with these types, but I'll merge now (in a bit) and figure it out later. I love these types. 👍

dancespiele commented 6 years ago

you right @jorgebucaran , the unique way to make compatible this h(nodeName, attributes, child1, child2, child3) or this: h(nodeName, attributes, [vnode, vnode, vnode], child2, ...) is:

export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes | null,
  ...children: any[],
): VNode<Attributes>

but children has to be like this ...children: any[] otherwise this for example wouldn't be possible

const App = h("main", null, [
    Wrapper({}, Text("Scoping CSS is hard")),
    Wrapper({}, Text("Not with styled components!")),
    Wrapper({color: "red"}, Button("I'm red!")),
]);
jorgebucaran commented 6 years ago

@dancespiele The reason h(nodeName, attributes, child1, child2, ...) exists is only to support JSX. Is there a way to make JSX not complain with your original type proposal?

dancespiele commented 6 years ago

I saw that this is the unique that I know which JSX not complain

export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes | null,
  ...children: any[],
): VNode<Attributes>

that makes possible this h(nodeName, attributes, child1, child2, ...) too

dancespiele commented 6 years ago

My last updated changed to this

export type Children = VNode | string | number | null;
export function h<Attributes>(
  nodeName: Component<Attributes> | string,
  attributes?: Attributes | null,
  ...children: Array<Children | Children[]>,
): VNode<Attributes>

I explain that. When you write a Arguments object typescript forces to write an array type but doesn't means that the type is an array, it means that every argument could be with the types that is inside of the array for example ...children: Array<VNode | string | number | null>; means that every children argument can be VNode | string | number | null but not this [VNode | string | number | null] the second way has to be typed like this ..children: Array<Array<VNode | string | number | null>> but we want that the children type has the two possibilities then you need to type like this Array<Array<VNode | string | number | null> | VNode | string | number | null> and to make it more legible it has to be like this

export type Children = VNode | string | number | null;

...children: Array<Children | Children[]>,

about this

attributes?: Attributes | null,

undefined is not the same that null then you need specify that otherwise wouldn't be possible to do this:

h("div", null, [...])
jorgebucaran commented 6 years ago

Thanks, @dancespiele! 🎉