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

fix flow typing | remove createSender API #7

Merged
merged 5 commits into from
Jul 22, 2018

Conversation

vovacodes
Copy link
Contributor

@vovacodes vovacodes commented Jul 21, 2018

Fixes #6

  • Requires Action instead of ActionType as a generic parameter of ReComponent and RePureComponent
  • makes sure the type of the action.payload is preserved throughout the component's API and reducer

Unfortunately, in order to ensure proper type-checking I had to remove createSender API.
I simply didn't manage to come up with typing that would ensure the correct payload typing, while maintaining the same API of createSender. Please let me know if you see a solution for it.

Even though it has to be removed, I don't think it would make the ergonomics of ReComponent significantly worse:

this.handleClick = this.createSender("CLICK");

becomes

this.handleClick = () => this.send({ type: "CLICK" });

or

this.handleClick = (clicks: number) => this.send({ type: "CLICK", payload: clicks });

NOTE: I did not fix documentation (remove all mentions of createSender and fix the examples) yet. I would like to wait for the code review first just to know if you are OK with this breaking change. If you decide to proceed with these changes I will make sure the docs are updated as well

@codecov
Copy link

codecov bot commented Jul 21, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #7   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          67     67           
  Branches       15     15           
=====================================
  Hits           67     67

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 e6eb7de...467ccd6. Read the comment docs.

Copy link
Owner

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Hey @wzrdzl!

Thank you so much for your contribution. I fully agree that we should require the type for an Action instead of an ActionType - this will dramatically improve the soundness of the typings. Great job!

I'm not sure about removing createSender I think it might still be useful in certain situations. I've pushed a commit to your repo that re-enables this method and adds the best typing that I could come up with. What do you think about that?

In addition to that, we'd probably want to publish this as a major release as this breaks existing types but I see no issue in that.

@vovacodes
Copy link
Contributor Author

vovacodes commented Jul 21, 2018

Hi @philipp-spiess ,

Thanks for such a quick reaction!

I agree that createSender is very convenient API and can definitely be useful, especially for someone who doesn't use flow.

What I would do though, is to update the documentation so that it's clear that you are losing the typing of payload if you use createSender API (payload is effectively typed as any). What do you think, shall I update it?

In addition to that, we'd probably want to publish this as a major release as this breaks existing types but I see no issue in that.

I agree, for any flow project these changes are breaking

@philipp-spiess
Copy link
Owner

It would be great if you could update the docs. I think we should remove createSender from the Flow examples and add a note as you said.

We should also roll back the tests I suppose so that we also test createSender.

Thank you for your help!

@vovacodes
Copy link
Contributor Author

Sure, no worries will update everything! :)

@vovacodes
Copy link
Contributor Author

@philipp-spiess I updated docs and brought back some this.createSender usages in the tests

Copy link
Owner

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

This is amazing! Thank you 👏 I'll release a 1.0.0-rc1 soon (tomorrow at the latest) so you can start using it.

@philipp-spiess philipp-spiess merged commit ef07fe9 into philipp-spiess:master Jul 22, 2018
@vovacodes vovacodes deleted the fix-flow-support branch July 22, 2018 09:55
@vovacodes
Copy link
Contributor Author

Thanks man, it's been a pleasure to collaborate with you :)

@philipp-spiess
Copy link
Owner

The pleasure is all mine 😊

FYI I've released 1.0.0-rc.1 with the next tag now. Be sure to check it out!

https://github.com/philipp-spiess/react-recomponent/releases/tag/v1.0.0-rc.1

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.

Improve Flow Coverage
2 participants