jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.06k stars 781 forks source link

Usage with TypeScript #1048

Open stewartrule opened 3 years ago

stewartrule commented 3 years ago

Hi, I'm running into some issues when using hyperapp (^2.0.13) with typescript (^4.2.3).

I remember trying things about about 6 months ago and had similar experiences but thought maybe the definitions were not really ready yet.

I'm not sure if there are any guidelines into usage with ts, but I kind of feel the generics aren't always passed down properly. I wrote down some issues below.


No problems yet.

import { app, h, text, View } from "hyperapp";
import { state, State } from "./state";

const Canvas: View<State> = () => {
  return h("div", {}, [text("ok")]);
};

app<State>({
  init: state,
  view: Canvas,
  node: document.getElementById("app")!,
});

Introducing .map(toShape) suddenly causes a conflict.

// Type '(state: State) => VDOM<unknown>' is not assignable to type 'View<State>'.
// Type 'VDOM<unknown>' is not assignable to type 'VDOM<State>'.
const Canvas: View<State> = (state) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h("h3", {}, [text(shape.type)]);
};

app<State>({
  init: state,
  view: Canvas,
  node: document.getElementById("app")!,
});

Removing View<State> and just using (state: State) moves the error up to the app call.

import { app, h, text, View } from "hyperapp";
import { ShapeType, state, State } from "./state";

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h("h3", {}, [text(shape.type)]);
};

app<State>({
  init: state,

  // Type '(state: State) => VDOM<unknown>' is not assignable to type 'View<State>'.
  // Call signature return types 'VDOM<unknown>' and 'VDOM<State>' are incompatible.
  view: Canvas,

  node: document.getElementById("app")!,
});

Passing State to every call to h seems to work, but I really don't want to do that.

import { app, h, text, View } from "hyperapp";
import { ShapeType, state, State } from "./state";

const Canvas = (state: State) => {
  return h<State>("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h<State>("h3", {}, [text(shape.type)]);
};

app<State>({
  init: state,
  view: Canvas,
  node: document.getElementById("app")!,
});

The only thing that seems to work ok is wrapping the h call itself and just hardcode S to State but since I just started looking into this I'm not sure if this won't cause other issues later on.

import { PropList, MaybeVDOM, VDOM, h as ha } from "hyperapp";
import { State } from "./state";

type H = <T extends string = string>(
  tag: T extends "" ? never : T,
  props: PropList<State>,
  children?: MaybeVDOM<State> | readonly MaybeVDOM<State>[]
) => VDOM<State>;

export const h: H = (tag, props, children) => ha(tag, props, children);

Any ideas on how to use hyperapp properly when using typescript?

zaceno commented 3 years ago

@stewartrule Hyperapp's type definitions are brand new and haven't been real-world-tested by enough people in enough scenarios yet, to really give you any good answers. Your input here is much appreciated and will help us polish things. Sorry that's the best answer I can give for now.

I can only chime in that I too have had a lot of issues with the generic type parameters to generic functions not being inferred correctly (becoming unknown after changes, seemingly at random).

@icylace might have more concrete help to give. I know he's working on a usage guide for the types but it is also still a WIP

stewartrule commented 3 years ago

@zaceno Thanks for your answer. I'm starting to think wrapping might be the only solution here since an import of h will probably never inherit the state passed to app to begin with. When using redux I usually also just wrap the hooks annotated with the actual state and action types (and import those) so everything is always typed correctly:

export const useSelector: TypedUseSelectorHook<State> = useReduxSelector;

maybe hyperapp could expose something similar so you could do:

export const h: TypedH<State> = originalH;
zaceno commented 3 years ago

Interesting idea!

TBH I'm kind of thinking the state inference down the entire vdom might not be worth it. If we got rid of it, it would open the possibility for mistakes of the kind where actions in different parts of the view are defined for different types of state. But I feel like that is going to be a pretty unlikely mistake, so the cost is not very high. The benefit would be that we don't get errors just because the inference failed for some reason.

icylace commented 3 years ago

@stewartrule Thank you for your input! I'll take a look at this when I have time later.

@zaceno It is worth it. "errors just because the inference failed for some reason" is important to me and my efforts to use Hyperapp in an enterprise setting. I'm sure I'm not the only one.

Besides, casting to any is always an escape hatch if it really comes down to it. Obviously, I want to mitigate that as much as possible. Also, I find JavaScript/TypeScript preferable to using Elm, ReasonML, PureScript, etc. for a few reasons. So, having the best possible TypeScript types is key for me.

icylace commented 3 years ago

@stewartrule Yeah, TypeScript's type inference is weird sometimes.

I'm still investigating this but another option you can do is:

import { app, h, text, View } from "hyperapp";
import { ShapeType, state, State } from "./state";

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h<State>("h3", {}, [text(shape.type)]);
};

app<State>({
  init: state,
  view: Canvas,
  node: document.getElementById("app")!,
});

Notice the h in toShape is the only one that needs the explicit type variable assignment.

icylace commented 3 years ago

@stewartrule I've never used React stuff, so thank you for mentioning TypedUseSelectorHook. It gave me something to study.

Please verify the following works on your end:

Add this new type definition toindex.d.ts of your Hyperapp folder within node_modules:

  interface TypedH<S> {
    <_, T extends string = string>(
      tag: T extends "" ? never : T,
      props: PropList<S>,
      children?: MaybeVDOM<S> | readonly MaybeVDOM<S>[]
    ): VDOM<S>
  }

and now hopefully this works for you:

import { app, h as ha, text, View } from "hyperapp";
import { ShapeType, state, State } from "./state";

const h: TypedH<State> = ha;

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h("h3", {}, [text(shape.type)]);
};

app<State>({
  init: state,
  view: Canvas,
  node: document.getElementById("app")!,
});

Let us know how it goes !

Edit: To be clear, I'm planning on turning this solution into a PR.

stewartrule commented 3 years ago

@icylace yep, that seems to work. I do think you can get rid of the _ since it isn't used?

icylace commented 3 years ago

@icylace yep, that seems to work. I do think you can get rid of the _ since it isn't used?

@stewartrule That should prevent some gotcha errors from happening. When I have time later I'll explain.

Edit: On second thought, maybe those errors would be beneficial.... hmm... will discuss later !

icylace commented 3 years ago

@stewartrule

TL;DR -- TypedH<S> will keep the _ but in an improved way.


Let's discuss _. Consider:

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h<State>("h3", {}, [text(shape.type)]);
};

versus:

const h: TypedH<State> = ha;

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h("h3", {}, [text(shape.type)]);
};

Both of them work. In the former, one h needs the <State> while in the latter none do. While it's true that using const h: TypedH<State> = ha; in every Hyperapp-related file of your app would work the cost is extra setup code.

Now, imagine a user who cares about this trade-off and is trying to decide one way or the other about what's best for their app as they work on it. They may experiment with including or removing const h: TypedH<State> = ha; in one or more files to find what's acceptable to them.

So, if TypedH<S> retains the placeholder _ type parameter as-is then the following is treated as valid:

const h: TypedH<State> = ha;

const Canvas = (state: State) => {
  return h("div", {}, state.shapes.map(toShape));
};

const toShape = (shape: ShapeType) => {
  return h<State>("h3", {}, [text(shape.type)]);
};

And if TypedH<S> doesn't have the _ then the aforementioned code is treated as invalid.

After weighing these options I've opted for correctness over convenience in this instance.

However, simply removing the _ misses an obscure and ridiculous edge case:

What if your state type was a string literal that exactly matched the tag name of the h call that has its type parameter explicitly assigned ?!

h<"h3">("h3", {}, [text(shape.type)])

Yeah, I know the odds of that are pretty unlikely but it's easily preventable while achieving the goal of increased correctness by replacing the _ with _ extends never.

So, I'm going with that.

icylace commented 3 years ago

@stewartrule The latest Hyperapp dot release, 2.0.15, contains TypedH! Please try it out and see if everything is alright on your end using it.

zaceno commented 3 years ago

@stewartrule I played around a little with your original example and this seems to work nicely (without the need for typing every h call):

import {h, text, app, VDOM} from 'hyperapp'

type State = {shapes: ShapeType[]}

type ShapeType = {type: string}

const Canvas = (state: State):VDOM<State> => h("div", {}, state.shapes.map(x => toShape(x)))

const toShape = <S>(shape: ShapeType):VDOM<S> => h("h3", {}, [text(shape.type)])

app({
  init: {shapes: []},
  view: Canvas,
  node: document.getElementById("app")!,
});

playground

Two things make this work (as far as I can tell):

As regards that last point, I think it is because typescript is expecting a function with the full mapper signature (with the second and third arguments typed as well), and when it gets a function that is explicitly typed differently, it throws off the inference somehow.

tillarnold commented 3 years ago

I hope this is the right place to ask/give feedback about the hyperapp type definitions.

The type definition for Effect currently is

 type Effect<S, P = any> = readonly [
    effecter: (dispatch: Dispatch<S>, payload: P) => void | Promise<void>,
    payload: P
  ]

While the documentation for Effect defines an Effect as [EffecterFn, Payload?]. So I was wondering if the typescript definition should be adapted to use payload?: P to make the payload optional.

icylace commented 3 years ago

@tillarnold Writing effects, or more specifically effecters, without a payload parameter should already be valid with the current definitions. That may sound strange. To find out why TypeScript does this, check this out: https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-with-fewer-parameters-assignable-to-functions-that-take-more-parameters

tillarnold commented 3 years ago

I'm not entirely sure but I think my previous comment was a bit impresise and I think the problem is not with the payload: P parameter to the effecter function but with the second element of the array. So my proposal was to define Effect as

  type Effect<S, P = any> = readonly [
    effecter: (dispatch: Dispatch<S>, payload: P) => void | Promise<void>,
    payload?: P
  ]

I propose this because with the current definition I get a typescript error with an Action like

export const FooBar = (state: State): Dispatchable<State> => [
  { ...state, abc: false },
  [
    (dispatch) => {
      doExternalThing();
    },
  ],
];
zaceno commented 3 years ago

@tillarnold I would open a new issue for this topic since it is technically a different question than the OP of this issue.

But anyway, I think you're right. "Inline effects" like this

import {Action} from 'hyperapp'

type State = {abc: boolean, def: number}

const FooBar : Action<State, any> = state => [
  { ...state, abc: false },
  [() => console.log('Hello!')],
]

...while perhaps not exactly "best practice" are technically allowed in plain javascript, and often useful in library code or or in early stages of development. As such the types should allow it, but as you point out they dont (see also playground),