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

Refactor typings to allow for better intellisense and support in Typescript #551

Closed

Conversation

alber70g
Copy link
Contributor

@alber70g alber70g commented Jan 8, 2018

As a continuation of this PR (#535 that might be gone...) I've created this PR.

!! Please have a look at this, anyone who has interest in Typings !!

I've changed the typings considerably and I need to polish them. In the mean time it would be nice to review them for what there is now.

A list of the changes:

  • all generics are prefixed with T to make the difference between types/interfaces and generics clear
  • I've added a generic NestedMap as helper type to form the other types (specifically State and Actions, and their derivatives)
  • I changed the exported 'app()' signature. Right now it's an implementation of an interface that's being exported (this allows @lassecapel to type the app that's passed into his HOA)
  • I've added WiredAction and UnwiredAction and their object wrappers as aliases so it's more convenient for implementing objects for respectively State/SubState and Actions/SubActions
  • I changed the signature of the generics so that the TPayload is always last and optional

Things to do:

  • fix the build (test/index.tsx in particular)
  • fix formatting according to Hyperapp standards
  • allow for nested types and actions to be in the same way accessible as the return value of App interface
  • change and move documentation in the proper place
  • allow for nested actions to get the nested state based on the path where the action is mounted
  • In progress: implement recommendations from review (please take a critical look at the current types and the uses of any as defaults for generics)
  • [discussion] there are now the concepts of UnwiredAction, WiredAction. I'm wondering if we should keep it this way. I feel that it gives good distinction between the two, but others think otherwise.
  • Not applicable anymore [discussion] a lot of generics are now typed optional with any, it might be better to have them non-optional

@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #551   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         133    132    -1     
  Branches       40     40           
=====================================
- Hits          133    132    -1
Impacted Files Coverage Δ
src/index.js 100% <0%> (ø) ⬆️

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 5291248...19aec4f. Read the comment docs.

</main>
)

app<Counter.State, Counter.Actions>(
const appActions = app<Counter.State, Counter.Actions>(
Copy link
Owner

Choose a reason for hiding this comment

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

main

Counter.state,
Counter.actions,
view,
document.body
)

appActions.up()
Copy link
Owner

Choose a reason for hiding this comment

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

main.up()

@@ -1,38 +1,49 @@
import { h, app, ActionsType, View } from "hyperapp"
import { h, app, View, StateType, UnwiredActions, UnwiredAction } from 'hyperapp'
Copy link
Owner

Choose a reason for hiding this comment

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

"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@alber70g alber70g changed the title WiP: refactor typings to allow for better intellisense and support in Typescript Refactor typings to allow for better intellisense and support in Typescript Jan 8, 2018
count: number
clickCount: number
Copy link
Owner

Choose a reason for hiding this comment

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

If you are going to use clickCount, then maybe change the counter example as well, because count and clickCount is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should I change it to?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, maybe come up with a different example. @Mytrill had a todo list originally, but I thought it was too long, so I replaced it with the current counter example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there so we can call this action in the up and down action to demonstrate that the types are working properly.

Copy link
Owner

Choose a reason for hiding this comment

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

If it's not a new example, then maybe just refactor the names.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, the test should have an example of asynchronous action (i.e. returning a promise) and an action where the return type is actually used by the view, since these could potentially be shortcomings of these types, which were not there with the previous version.

down(): State
up(value: number): State
export interface Actions extends UnwiredActions<State, Actions> {
down: UnwiredAction<State, Actions>
Copy link
Owner

Choose a reason for hiding this comment

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

UnwiredActions is rather long. What's wrong with ActionsType or ActionsTemplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, just wanted to put something that's not even remotely confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm for changing UnwiredAction to ActionsTemplate. But what about WiredActions? It's an internal type that no-one should be using anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

I see. Don't change it yet, please.

TActions extends UnwiredActions<TState, TActions>,
TPayload extends Primitive | NestedMap<Primitive> = any
> = (
data?: TPayload
Copy link
Owner

Choose a reason for hiding this comment

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

What is TPayload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generic to type the payload

Copy link
Owner

Choose a reason for hiding this comment

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

What is the payload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The payload passed to an action when calling from the view or inside an action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redux also calls it payload and it's called payload in the Flux architecture. I believe it's wise to keep it the same as the rest of the community calls it.

Copy link
Owner

Choose a reason for hiding this comment

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

@alber70g Ah, the data?

actions.foo(payload)

?

Choose a reason for hiding this comment

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

Yes the Data, but I think payload makes sense since it is used as a name in Redux too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's correct.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with this! Thanks for explaining.

@jorgebucaran jorgebucaran added the types Strings, numbers, booleans label Jan 8, 2018
@jorgebucaran
Copy link
Owner

Do you have any links that I can start reading to help me understand this PR? I don't expect to understand everything about TS in one sit, but at least something that can help figure out what's going on here.

@alber70g
Copy link
Contributor Author

alber70g commented Jan 8, 2018

Start reading advanced types

Also, to see the power of typings, open the test/index.tsx in VSCode and try to add an action in jsx or referencing the state. You'll see the typings are nice and helping the developer.

@jorgebucaran
Copy link
Owner

@alber70g Any way we can simplify this?

@alber70g
Copy link
Contributor Author

alber70g commented Jan 8, 2018

@jorgebucaran Why and what would need to be simplified? The reason it's complex is because we want to pass a type that defines the actions as UnwiredActions but want to use them as WiredActions.

const actions = {
  up: () => (state, actions) => ({})
}

I'm talking about (state, actions) => ({}). Here the actions we pass to this function aren't the original actions.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jan 8, 2018

@alber70g What would make the types simpler? It would have been nice we had this convo before the 1.0. Do you find the current API particularly hard to type? or would it be just as hard if we had a different actions signature?

Why?

Because simpler is better than hard of course! :)

@alber70g
Copy link
Contributor Author

alber70g commented Jan 8, 2018

@jorgebucaran I tried to change stuff before the 1.0 release but there was no attention for the typings at all. No one except me uses Hyperapp with TypeScript right now. To make things less complex, make Hyperapp simpler. The current typings represent the API of Hyperapp.

Also, the definition might be complex, but using them is easy as you can see in the test/ts/index.tsx

Ping @lassecapel: have a look at the typings, see if they're good for your redux devtools implementation

actions.addClickCount()
return { count: state.count + value }
},
addClickCount: () => state => ({ clickCount: state.clickCount + 1 }),
}
}

const view: View<Counter.State, Counter.Actions> = (state, actions) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

The View<...>type accepts WiredActions<...>, but here you pass it Actions which are of type UnwiredActions<...>, so I have 2 questions:

  • does this actually compiles?
  • inside the view, the user will call an unwiredActions as if it was a wired action? Shouldn't the type of View be:
export interface View<
  TState extends StateType,
  TActions extends WiredActions<TState, TActions>
> {
  // Use the proxy here
  (state: TState, actions: ActionProxy<TState, TActions>): VNode<any>
}

Other than this, neat changes! With the new API it makes sense now for users to type the unwired actions and get the wired action type auto generated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's a bit strange. But it does compile somehow... I'll investigate more :) Thanks for your review 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I've changed the signature of the View. But in your snippet the TActions extends WiredActions<..., ...> should be TActions extends UnwiredActions<..., ...> because you can only proxy UnwiredActions -> WiredActions. To see that it's correct now, you can look at the index.tsx and see what signature the view's onclick={() => actions.up(5)}: (property) up: (data?: any) => Counter.State. This is correct.

The reason why it compiles is because the UnwiredActions' TActions generic is ambiguous because it's being proxied to WiredActions :')

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice!

Copy link

@lassecapel lassecapel left a comment

Choose a reason for hiding this comment

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

I like the more explicit typings especially the difference in wired and unwired actions types. The names could be discussed.

TActions extends UnwiredActions<TState, TActions>,
TPayload extends Primitive | NestedMap<Primitive> = any
> = (
data?: TPayload

Choose a reason for hiding this comment

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

Yes the Data, but I think payload makes sense since it is used as a name in Redux too.

/**
* Convenience type for NestedMap<WiredAction>
*/
interface WiredActions<

Choose a reason for hiding this comment

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

Since you implement this. It would be nice to use Actions here. You can then use ActionsTemplate then for the UnwiredActions

Choose a reason for hiding this comment

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

Another proposal: Actions and AppActions. Where Actions are the unwired ones and AppActions the wired ones

Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to use Actions for the most common one, yeah.

Copy link
Contributor Author

@alber70g alber70g Jan 9, 2018

Choose a reason for hiding this comment

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

What is 'most common' where you implement actions, or where you use them (e.g. in actions and in the view function)?

/**
* The interface for app(). Use this for implementing Higher Order App's
*/
export interface App {

Choose a reason for hiding this comment

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

This would work for the redux-devtools HOA

) =>
| ((state: State, actions: Actions) => ActionResult<State>)
| ActionResult<State>
type ActionProxy<

Choose a reason for hiding this comment

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

Nice way to define the wiring. This was causing me some trouble understanding this out of the documentation.

@alber70g
Copy link
Contributor Author

alber70g commented Jan 8, 2018

@jorgebucaran (& @Mytrill) I've found something that can be simplified in a way that you don't need to type the object containing the UnwiredActions, like so:

type UnwiredActions<
TState extends StateType,
TActions extends UnwiredActions<TState, TActions>
> = { [P in keyof TActions]: NestedMap<UnwiredAction<TState, TActions>> }

Edit:

type UnwiredActions<
  TState extends StateType,
  TActions extends UnwiredActions<TState, TActions>
> = { [P in keyof TActions]: UnwiredAction<TState, TActions> }

I'm still in a thinking process, but I thought I could share it. I'll update the PR when I'm ready.

@alber70g
Copy link
Contributor Author

alber70g commented Jan 8, 2018

It's still not perfect. You cannot add nested actions in the actions object. This disallows the use of slices. Also we need to figure out a way to declare nested types and infer their type. Not sure if that's possible right now, but we can provide a 'helper' that allows for a certain level of nesting: microsoft/TypeScript#10693

Although this is possible, it's far from perfect since the View doesn't know about nested actions now (in the current state)...

export interface Actions {
  down: UnwiredAction<State, Actions>
  up: UnwiredAction<State, Actions, number>
  nested: {
    addClickCount: UnwiredAction<State['nested'], Actions>
  }
}

But that can be fixed by this change:

type ActionProxy<
  TState extends StateType,
  TActions extends UnwiredActions<TState, TActions>
> = { [P in keyof TActions]: TActions[P] }
type ActionProxy<..., ...,> = { [P in keyof TActions]: WiredAction<TState, any> } 

@Swizz Swizz self-requested a review January 8, 2018 19:47
@alber70g
Copy link
Contributor Author

alber70g commented Jan 9, 2018

I'm a bit stuck right now. The problem I face is that whenever you create a nested state and nested action, the type of the nested UnwiredAction has a state and actions passed living on the same path as the UnwiredAction. I don't know how to define that state... somehow, in the types, there needs to be a connection between state and actions. This isn't possible with two generics.

If anyone would like to show me some suggestions, I might be able to integrate it.

@Mytrill
Copy link
Contributor

Mytrill commented Jan 9, 2018

I used something like this before, and it seemed to work:

type ActionProxy<TState extends Partial<Record<keyof TActions, any>>, TActions> = {
  [P in keyof TActions]: WiredAction<TState[P], TActions[P]>
}

The Partial<Record<keyof TActions, any> makes sure that TState and TActions have some keys in common, and allows to write TState[P].

Hope that helps :)

@alber70g
Copy link
Contributor Author

Sorry for the radio silence the past few weeks. I've been working on fixing this problem on and off, and in the mean time I've been ill.

This issue of the nested types can be fixed for good once conditional types (microsoft/TypeScript#21316) are landing in TypeScript.

This solution (microsoft/TypeScript#12424 (comment)) can be used in the mean time. I haven't started experimenting with it yet, but I'll try that this weekend.

The final way to solve this issue is to not type it (actions) at all, and just use any. This means however, that we cannot have inferred typings for the view and the actions parameter in the actions, unless you pass them explicitly as a generic.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 16, 2018

@alber70g This is great work. Any plans to continue this PR. 🤔

@alber70g
Copy link
Contributor Author

alber70g commented Mar 12, 2018

Right now it can be used as is, but it'll not be flawless with slices. I'd like to wait until conditional types land in Typescript 2.8 which will be released soon I think.

So for the time being, let's leave this PR open so I can continue on this one afterwards

@alber70g
Copy link
Contributor Author

alber70g commented Mar 21, 2018

I've done some digging with typescript@2.8 alpha, and its not working in a way I want it to be.

There's no way to infer types and use the children of a different nested type (i.e. how slices work in HA) in another type.
In other words: you cannot make the nested type of the Actions refer to the nested type of State in order to type the State that's being passed to a particular Action living on the same 'slice'.

Example (inline comments):

app({ counter: 0, nested: { counter: 0} }, 
{
  inc: () => (state) => ({ counter: state.counter + 1 }),
  nested: {
    inc: () => (nestedState) => ({ counter: nestedState.counter + 1 }) 
    // currently the typings do not support Typescript in figuring out 
    // that this "nestedState" is the "state.nested".
  }
}

I will finalise this pull request in the coming week.

@jorgebucaran
Copy link
Owner

@alber70g ...currently the typings do not support Typescript in figuring out that this "nestedState" is the "state.nested".

Is this also true in TS 2.8?

@alber70g
Copy link
Contributor Author

alber70g commented Mar 26, 2018

@jorgebucaran Yes

typescript@2.8 alpha

You cannot use a second type as a path in the first type.

@jorgebucaran
Copy link
Owner

😢

@jorgebucaran
Copy link
Owner

With 2.0 we won't need most of this work, so I am going to close here. We'll need types though! But I imagine you'll prefer to create a new PR for that then? 😅

Closing.

@alber70g
Copy link
Contributor Author

alber70g commented Apr 9, 2018

That's fine 👌 Once the API of 2.0 is clear I can start working on that one

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

Successfully merging this pull request may close these issues.

None yet

4 participants