Skip to content

Typescript Typings #9

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

Merged
merged 17 commits into from
Aug 17, 2019
Merged

Typescript Typings #9

merged 17 commits into from
Aug 17, 2019

Conversation

sandorfr
Copy link
Contributor

@sandorfr sandorfr commented Jun 18, 2019

This adds typescript typings to the package. This would enable typescript support out of the box.
TODO:

  • Add typing tests
  • Add typescript examples?
  • Make typescript type names consistent with flow names as much as possible

@sandorfr sandorfr requested a review from albertogasparin June 18, 2019 03:04
@albertogasparin
Copy link
Collaborator

Thanks for this!! I was about to tell you that I tried the conversion of the entire lib to typescript in the branch feture/typescript(typo...) but typing the actions binding internally had been more complicated than expected....
So probably this can be a good ad interim solution :)

@albertogasparin albertogasparin requested a review from meandmax June 21, 2019 01:11
- Fixed options type for createSubscriber
- Fixed options type for createHook
- Fixed onUpdate method type for createContainer
@ferborva
Copy link
Contributor

Missing type for defaults object

@albertogasparin
Copy link
Collaborator

Hope you don't mind @sandorfr if I've pushed my commit here, it just makes our life easier having one "source of truth". It is still WIP but I've reordered the types so they match Flow (much easier to change/fix stuff if they are almost 1:1) and added the same tests that I did for Flow.
It mostly works except the return type of the bound actions.

Was not able to find a way, given actions { a: (...) => ({}) => ar, b: (...) => ({}) => br } to map the object and get in the RPC/Hooks the type as { a: (...) => ar, b: (...) => br }.
So right now the actions in the components still return the inner thunk function instead of it's return value 😞

Ideas on how we can achieve that?

@sandorfr
Copy link
Contributor Author

@albertogasparin Of course I don't mind, quite the opposite. I'll try to have a look later today.

@sandorfr
Copy link
Contributor Author

@albertogasparin The thunks should look better now. Also you removed the constraint on the shape of Actions when creating a store. which means we would accept anything. I've reintroduced it, let me know if it triggers question

@albertogasparin
Copy link
Collaborator

Great! We are almost there. There are just 2 things left to fix

@sandorfr
Copy link
Contributor Author

@albertogasparin is there a way to disable the flow checks and lint rules for the tsx files :D It's a bit confusing 😭

@albertogasparin
Copy link
Collaborator

Done. Run npm install and you should get eslint correctly checking. I looked at Flow but it does not complain for me on TS files (however if you get TS errors on flow files you should set "javascript.validate.enable": false in the workspace config).

@sandorfr
Copy link
Contributor Author

sandorfr commented Jul 1, 2019

thanks @albertogasparin that was just esling which had flow rules (confused me)

On the inference, I'd like this to work:

const inferedStore = createStore({
  initialState: {
    counter : 0
  },
  actions: {
    increment: (by: number = 1) => ({getState, setState}) =>{
      setState({
        counter : getState().counter + by
      })
    },
    composite : () => ({actions}) => {
      actions.increment(2);
    }
  }
});

Which currently fails with TS2339:

Property 'increment' does not exist on type 'BoundActions<{ counter: number; }, unknown>'.

I'm not sure it's fixable :( I'm afraid we are running in a case related to microsoft/TypeScript#6230

add test for full inference on createStore
@albertogasparin albertogasparin mentioned this pull request Aug 5, 2019
@cytrowski
Copy link

Speaking of types testing you should consider adding dtslint for that. I have some POC setup in https://github.com./cytrowski/teedux/pull/3/files

@albertogasparin albertogasparin changed the title Typescript Typings draft Typescript Typings Aug 17, 2019
@albertogasparin albertogasparin marked this pull request as ready for review August 17, 2019 01:17
@albertogasparin
Copy link
Collaborator

Thanks @cytrowski, updated to use dtslint

@albertogasparin albertogasparin merged commit 467ab96 into master Aug 17, 2019
@albertogasparin albertogasparin deleted the feature/typescript-typings branch August 17, 2019 02:23
@albertogasparin
Copy link
Collaborator

Closes #29

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.

4 participants