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

Improve Flow types #232

Closed
wants to merge 2 commits into from

Conversation

migueloller
Copy link

@migueloller migueloller commented Oct 26, 2018

Adds better Flow support. Now, if the return type of the recipe never returns undefined, it can be guaranteed that produce won't return undefined. This allows compatibility with other types, like Redux's Reducer type.

i.e.,

import type { Reducer } from 'redux'

type State = {| count: number |}

type Action = {| type: 'INCREMENT' |} | {| type: 'DECREMENT' |}

const reducer: Reducer<State, Action> = (state: State = { count: 0 }, action) => {
  switch(action.type) {
    case 'INCREMENT':
      state.count++
      break

    case 'DECREMENT':
      state.count--
  }

  return state
}

// This is now possible! Before it would fail, as the return type of
// produce would be `State | void`.
const initialState: State = produce(reducer)(undefined, { type: 'INCREMENT' })

@migueloller
Copy link
Author

migueloller commented Oct 26, 2018

Seems to be failing tests that fail in master as well. Looks like #231 fixes it. Will rebase once #231 is merged.

@aleclarson
Copy link
Member

You shouldn't need to rebase, since you're not touching any of the same files as #231

@aleclarson aleclarson self-assigned this Oct 26, 2018
@aleclarson aleclarson added this to the 1.7.4 milestone Oct 26, 2018
@migueloller
Copy link
Author

migueloller commented Oct 27, 2018

@aleclarson, sweet! Let me know if there's anything else you need from me 😄

@aleclarson aleclarson modified the milestones: 1.7.4, 1.7.5 Oct 27, 2018
@aleclarson
Copy link
Member

aleclarson commented Oct 27, 2018

@mweststrate won't be maintaining much in the next 2 weeks, and we need him to review this PR. Therefore, I moved this PR to v1.7.5 instead of v1.7.4.

@mweststrate
Copy link
Collaborator

The CI failed, I guess this still needs rebasing?

Copy link
Collaborator

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

  • Flow type updates look correct to me, approved that part
  • Build is failing, needs to be rebased on master?
  • The changes in this MR are more than just the flow typings, was this PR based on a not-yet-merged branch? If so, we need to make sure that one get's first

@aleclarson
Copy link
Member

@mweststrate This PR was put up before I removed failing tests from master with a rebase (naughty, I know).

@migueloller
Copy link
Author

@mweststrate, thanks for the review! Rebasing now.

@coveralls
Copy link

coveralls commented Nov 8, 2018

Coverage Status

Coverage remained the same at 96.393% when pulling 46d9db7 on migueloller:flow-type-definitions into 4613b06 on mweststrate:master.

@mweststrate
Copy link
Collaborator

@migueloller, some tests are failing in the CI. Probably locally as well. Could you look into this?

@migueloller
Copy link
Author

@mweststrate, I spent a bit looking at the failing test and realized a few things.

  • There are other regressions on the Flow types
  • $ExpectError tests weren't even working at all
  • The currently proposed types seem to break down in some use cases

I want to spend some more time to address these issues but I'm extremely busy with my day job at the moment. I will push new changes ASAP.

Flow will now include warnings when running (i.e., unused supression
comments). Flow will also fail if there are any warnings. This fixes
broken Flow tests.
Adds better Flow support. Now, if the return type of the recipe never
returns `undefined`, it can be guaranteed that `produce` won't return
`undefined`. This allows compatibility with other types, like Redux's
`Reducer` type.

i.e.,

```js
type Reducer<S, A> = (S | void, A) => S

declare var reducer: Reducer<State, Action>

// This is now possible! Before it would fail, as the return type of
// produce would be `State | void`.
const initialState: State = produce(reducer)(undefined, action)
```
@mweststrate mweststrate self-assigned this Feb 3, 2019
@mweststrate mweststrate mentioned this pull request Feb 3, 2019
5 tasks
@mweststrate
Copy link
Collaborator

Merged into 3.0 branch

@migueloller migueloller deleted the flow-type-definitions branch April 15, 2019 14:15
@migueloller migueloller restored the flow-type-definitions branch April 15, 2019 19:10
@mweststrate mweststrate reopened this Apr 15, 2019
@mweststrate
Copy link
Collaborator

Undid that :)

@mweststrate
Copy link
Collaborator

Closing for inactivity

@mweststrate mweststrate closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants