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

Usage with TypeScript #1048

Open
stewartrule opened this issue Apr 7, 2021 · 15 comments
Open

Usage with TypeScript #1048

stewartrule opened this issue Apr 7, 2021 · 15 comments
Labels
question How do I do that? types Strings, numbers, booleans

Comments

@stewartrule
Copy link

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?

@jorgebucaran jorgebucaran added question How do I do that? types Strings, numbers, booleans labels Apr 7, 2021
@zaceno
Copy link
Contributor

zaceno commented Apr 7, 2021

@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
Copy link
Author

@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
Copy link
Contributor

zaceno commented Apr 7, 2021

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
Copy link
Contributor

icylace commented Apr 7, 2021

@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
Copy link
Contributor

icylace commented Apr 7, 2021

@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
Copy link
Contributor

icylace commented Apr 7, 2021

@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
Copy link
Author

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

@icylace
Copy link
Contributor

icylace commented Apr 9, 2021

@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 icylace mentioned this issue Apr 11, 2021
@icylace
Copy link
Contributor

icylace commented Apr 11, 2021

@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.

  • With _ pros: More lenient towards varying h. Therefore potentially less coding friction.
  • With _ cons: Explicit type parameter assignments could be anything and no errors would be given.
  • Without _ pros: Better protection against falsely validating a meaningless type parameter assignment.
  • Without _ cons: Less lenient towards varying h. Therefore potentially more coding friction.

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
Copy link
Contributor

icylace commented Apr 18, 2021

@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
Copy link
Contributor

zaceno commented Apr 19, 2021

@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):

  • Being explicit about the return values of components (VDOM<State> and VDOM<S>)
  • Using x => toShape(x) in map, rather than just toShape

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
Copy link

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
Copy link
Contributor

icylace commented May 25, 2021

@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
Copy link

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
Copy link
Contributor

zaceno commented May 25, 2021

@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),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question How do I do that? types Strings, numbers, booleans
Projects
None yet
Development

No branches or pull requests

5 participants