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

Better Integration for TypeScript + Preact + Unistore #1579

Closed
wants to merge 1 commit into from

Conversation

yokotaso
Copy link

I write PoC program with TypeScript, Preact, Unistore(Preact & Unistore is great!)

I happened TypeScript error. Because type definition of onClick is like below(in jsx.d.ts):

onClick?: MouseEventHandler;
type MouseEventHandler = EventHandler<MouseEvent>;
interface EventHandler<E extends Event> {
		(event: E): void;
}

So onClick accepts function that has 1 argument, and the type is MouseEvent.
But Integrate with unistore, first argument of function is State object.

TypeScript compile error:
jsxerror

Sample code is here: https://github.com/yokotaso/preact-unistore-typescript-integration

I suggest to add UnistoreEventHandler for unistore.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6f67a04 on yokotaso:jsx-unistore into 5cac157 on developit:master.

@@ -273,24 +273,30 @@ export namespace JSXInternal {
interface PathAttributes {
d: string;
}

type EventHandlers<E extends Event> = EventHandler<E> | UnistoreEventHandler<any, E>;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your PR 🎉 I'm honestly uneasy with adding library specific code to preact. If we start with it soon others will follow and we'll be forced to maintain it. Maybe there is a way to fix this in the typings for unistore? That would be a lot cleaner 🙂

@ForsakenHarmony
Copy link
Member

As Marvin said, this is the wrong place, we can't add library specific typings

One solution to the problem would be to use props.increment.bind(null, null)

But your typings for the props are incorrect as well, the function doesn't take the state as an argument, that gets added by unistore

the correct type would be increment(): void;, as it doesn't return the update either

@yokotaso
Copy link
Author

@marvinhagemeister @ForsakenHarmony
Thank you. I readlized that I misunderstood type of unistore

@yokotaso yokotaso deleted the jsx-unistore branch April 28, 2019 05:59
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.

None yet

4 participants