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

TypeScript definitions for 2.0.8 #969

Closed
wants to merge 50 commits into from
Closed

TypeScript definitions for 2.0.8 #969

wants to merge 50 commits into from

Conversation

icylace
Copy link
Contributor

@icylace icylace commented Jul 21, 2020

Here is an update to the TypeScript definitions I've made for Hyperapp.

Suggestions always welcome !

index.d.ts Outdated Show resolved Hide resolved
Copy link

@gamebox gamebox left a comment

Choose a reason for hiding this comment

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

Looks pretty good, besides the comments I have given.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
hanneskaeufler and others added 2 commits July 25, 2020 21:43
While the variable name is true in what it describes, I feel it's improved by naming it for what it _means_.
Nitpick: Rename constant for clarity.
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
xieyuheng and others added 2 commits July 28, 2020 12:35
@hlibco
Copy link

hlibco commented Jul 30, 2020 via email

icylace and others added 7 commits August 17, 2020 18:18
`Transition` and `StateWithEffects` (especially the latter) were created to make typing the return types of actions easier to do. The benefit they provide is best seen when working with actions that return more than just an updated state.

As a bonus they allowed the type definitions here to be further streamlined.
Minor documentation fixes; remove mention of payload filters, use `text` in examples.
@icylace icylace marked this pull request as draft September 2, 2020 22:59
@icylace icylace marked this pull request as ready for review September 3, 2020 05:56
index.d.ts Outdated Show resolved Hide resolved
@icylace icylace marked this pull request as draft September 5, 2020 14:58
@zaceno
Copy link
Contributor

zaceno commented Jan 15, 2021

@icylace With the following definition of h we can produce an error if someone accidentally omits the properties argument

function h<S, T>(
    type: string,
    props: T extends VDOM<S> ? never : T & PropList<S>,
    children?: VNode<S> | readonly VNode<S>[],
): VDOM<S>

demo:

Screenshot 2021-01-15 at 12 09 45

@zaceno
Copy link
Contributor

zaceno commented Jan 16, 2021

@icylace I discovered another issue with the types as they are at the moment. It seems that init is not assignable to anything that is dispactchable (which it shoudl be). For instance, if you want to run an effect at that start of your app, you will typically have:

app({
  init: [
    {initial: 'state'},
    initalEffect('http://foo.com')
  ],
  ...
})

But it looks like that is not allowed by the types even though it is a fully legal thing to dispatch.

Screenshot 2021-01-16 at 21 14 43

@icylace
Copy link
Contributor Author

icylace commented Jan 19, 2021

@zaceno Your suggestion for h fixes one problem but somehow fails an existing test:

h("p", { style: { clor: "hi" } })          // $ExpectError

style shouldn't allow nonexistent CSS properties.

I'm trying to address this in accordance with your suggested change but it's tricky. If you have any ideas, that'd be great !

@zaceno
Copy link
Contributor

zaceno commented Jan 19, 2021

@icylace

Sorry I don't know TS well enough yet. I don't even understand how that error could have come up, let alone how to fix it 😅

I guess it means T & PropList<S> is somehow less restrictive than just PropList<S> – but how can that be?!

What about other restrictions on PropList? This should throw an error too I think, but does it? :

h("p", {key: 42})

@jorgebucaran
Copy link
Owner

Isn't the point of types to help you catch errors before they happen? Why not also forbid going against best practice?

What are types init can be but shouldn't?

@zaceno
Copy link
Contributor

zaceno commented Jan 19, 2021

@jorgebucaran I think enforcing "best practice" with types is going to be bad for people who use typescript to write and compile their apps. Maybe that's what a linter is for (warnings without actually breaking your code).

What are types init can be but shouldn't?

I can't think of anything off-hand. Are you saying having an effect immediately on start up is a bad idea?

@jorgebucaran
Copy link
Owner

Agreed. 👍

As for init, the ideal would be an action such as Initialize or anything that makes sense to you. Init is an action itself, so an anonymous function is good too.

@icylace
Copy link
Contributor Author

icylace commented Jan 22, 2021

@zaceno I just updated the types to fix false positives for PropList. It was really tricky for a while !

@icylace icylace changed the base branch from master to main January 24, 2021 06:52
@icylace icylace changed the base branch from main to master January 24, 2021 06:53
@icylace icylace changed the base branch from master to main January 24, 2021 07:06
@icylace icylace closed this Jan 24, 2021
@icylace icylace deleted the master branch January 24, 2021 07:16
@icylace icylace restored the master branch January 24, 2021 07:34
@icylace icylace reopened this Jan 24, 2021
@icylace icylace closed this Jan 24, 2021
@icylace icylace deleted the master branch January 24, 2021 07:38
@icylace icylace restored the master branch January 24, 2021 07:47
@icylace
Copy link
Contributor Author

icylace commented Jan 24, 2021

New and improved PR coming soon !

@icylace
Copy link
Contributor Author

icylace commented Jan 24, 2021

Please go here for the new PR: #1016

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.