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

remove cross type reference by direct import, fix broken tests #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quanlinc
Copy link
Owner

@quanlinc quanlinc commented Nov 15, 2019

@LoganBarnett
Copy link

I big part about getting these reviews streamlined is documentation and explanation. Seeing what you have in the pull request description would be really helpful, and I would add surrounding comment blocks that indicate the types you have are support types copied from redux. Bonus points if you reference the ticket that talks about this issue, so it's easy for others to track down where we've applied workarounds, should we ever develop a fix. You can find the ticket here: flow-typed#1857

@quanlinc
Copy link
Owner Author

quanlinc commented Nov 16, 2019

@LoganBarnett I omitted the step of providing links to documentation and github repo as I was opening multiple PRs to my own repo, not yet pushing out to original flow-typed repo, but all you are saying are well accepted. Will add some comment to that and thanks for the ticket link.

@LoganBarnett
Copy link

I think Github will let you retarget the pull request. In the worst case you could go to edit the PR description and copy+paste into a new one :) Either way I suggest writing the docs mentioned so you can get the full benefit of this pre-review we're doing.

@gyrfalcon
Copy link
Collaborator

Any sort of testing we could put in place for this? Other than that, looks good to me.

@quanlinc
Copy link
Owner Author

quanlinc commented Nov 20, 2019

Don't know if we need to come up with some test cases for a different way of 'import', those type definitions from where they are copied from 'should' be covered with some test cases already, @LoganBarnett any thoughts on Justin's suggestion?

@LoganBarnett
Copy link

To test this (unsure if this would be caught in flow-typed) is to attempt to use the built-in redux types (even if they are the copied versions) incorrectly, and expect an error. If even the simplest case fails to... fail, the type must be any - and that any is what we are trying to prevent.

Does that make sense?

@gyrfalcon
Copy link
Collaborator

Also, to be clear, I'm not saying we need to have tests for this. Given the nature of the issue it seems like it could be more complex to test this than it's worth, but having a discussion about the viability of it seemed useful. 😃

Copy link

@LoganBarnett LoganBarnett left a comment

Choose a reason for hiding this comment

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

Let's get some tests in place that assures we are not getting an any back from any of the Redux types.

Copy link

@LoganBarnett LoganBarnett left a comment

Choose a reason for hiding this comment

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

Left comments asking for any tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants