Updates for 0.12

[natefaubion]

Also includes some renames for consistency.

[natefaubion]

Are we interested in adding the new lifecycle methods?

[ethul]

Thanks for putting together this PR. I think adding the new lifecycle methods would be great. Apologies on the delay on a release. I will create a release after this PR.

On Sat, Jun 2, 2018 at 16:23 Nathan Faubion notifications@github.com wrote:

Are we interested in adding the new lifecycle methods?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/purescript-contrib/purescript-react/pull/149#issuecomment-394114630, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVYy2qXZdoNDT0QI9Bx1dlrbTa19sf-ks5t4vQqgaJpZM4UV7UO .

[natefaubion]

One thing that's weird is the interplay between getSnapshotBeforeUpdate and componentDidUpdate. https://reactjs.org/docs/react-component.html#getsnapshotbeforeupdate Both are optional, but if you provide getSnapshotBeforeUpdate, then the return value will be provided to componentDidUpdate. Not sure of the best way to encode this. One option is

foreign import data ReactUnusedSnapshot :: Type

-- | Required fields for constructing a ReactClass.
type ReactSpecRequired state r =
  ( state :: state
  , render :: Render
  | r
  )

type ReactSpecUnsafe props state r =
  ( unsafeComponentWillMount :: ComponentWillMount
  , unsafeComponentWillReceiveProps :: ComponentWillReceiveProps props
  , unsafeComponentWillUpdate :: ComponentWillUpdate props state
  | r
  )

-- | Optional fields for constructing a ReactClass.
type ReactSpecOptional props state snapshot r =
  ( componentDidMount :: ComponentDidMount
  , componentDidCatch :: ComponentDidCatch
  , componentWillUnmount :: ComponentWillUnmount
  | ReactSpecSnapshot props state snapshot r
  )

type ReactSpecSnapshot props state snapshot r =
  ( componentDidUpdate :: ComponentDidUpdate props state snapshot
  , getSnapshotBeforeUpdate :: GetSnapshotBeforeUpdate props state snapshot
  | r
  )

type ReactSpecShouldComponentUpdate props state =
  ( shouldComponentUpdate :: ShouldComponentUpdate props state
  )

type ReactSpecAll props state snapshot
  = ReactSpecRequired state
  + ReactSpecOptional props state snapshot
  + ReactSpecShouldComponentUpdate props state

component
  :: forall props state snapshot r spec
   . Row.Union
      (ReactSpecRequired (Record state) r)
      (ReactSpecAll (Record props) (Record state) ReactUnusedSnapshot)
      spec
  => Row.Nub spec (ReactSpecAll (Record props) (Record state) snapshot)
  => String
  -> ReactClassConstructor (Record props) (Record state) r
  -> ReactClass (Record props)

This type signature is kind of bonkers, but the inference is actually decent.

testChildren :: ReactClass { label :: String, children :: Children }
testChildren = component "TestChildren" \this -> do
  let
    getSnapshotBeforeUpdate _ _ = do
      pure { foo: 42 }

    render = do
      props <- getProps this
      pure $ div'
        [ text props.label
        , div' (childrenToArray props.children)
        ]
  pure
    { state: { a: "start", b: 0 }
    , render
    , getSnapshotBeforeUpdate
    }

  Could not match type

    { foo :: Int
    }

  with type

    ReactUnusedSnapshot

while trying to match type t2
                             { foo :: Int
                             }
  with type Effect ReactUnusedSnapshot
while solving type class constraint

  Prim.Row.Nub t0
               ( state :: { a :: String
                          , b :: Int
                          }
               , render :: Effect ReactElement
               , componentDidMount :: Effect Unit
               , componentDidCatch :: Error
                                      -> { componentStack :: String
                                         }
                                         -> Effect Unit
               , componentWillUnmount :: Effect Unit
               , componentDidUpdate :: { label :: String
                                       , children :: Children
                                       }
                                       -> { a :: String
                                          , b :: Int
                                          }
                                          -> t1 -> Effect Unit
               , getSnapshotBeforeUpdate :: { label :: String
                                            , children :: Children
                                            }
                                            -> { a :: String
                                               , b :: Int
                                               }
                                               -> Effect t1
               , shouldComponentUpdate :: { label :: String
                                          , children :: Children
                                          }
                                          -> { a :: String
                                             , b :: Int
                                             }
                                             -> Effect Boolean
               )

while inferring the type of component "TestChildren"
in value declaration testChildren

where t0 is an unknown type
      t1 is an unknown type
      t2 is an unknown type

And adding a corresponding componentDidUpdate works and infers correctly.

testChildren :: ReactClass { label :: String, children :: Children }
testChildren = component "TestChildren" \this -> do
  let
    componentDidUpdate :: _
    componentDidUpdate _ _ _ = do
      pure unit

    getSnapshotBeforeUpdate _ _ = do
      pure { foo: 42 }

    render = do
      props <- getProps this
      pure $ div'
        [ text props.label
        , div' (childrenToArray props.children)
        ]
  pure
    { state: { a: "start", b: 0 }
    , render
    , getSnapshotBeforeUpdate
    , componentDidUpdate
    }

  Wildcard type definition has the inferred type

    { label :: String
    , children :: Children
    }
    -> { a :: String
       , b :: Int
       }
       -> { foo :: Int
          }
          -> Effect Unit

  in the following context:

    getSnapshotBeforeUpdate :: { label :: String
                               , children :: Children
                               }
                               -> { a :: String
                                  , b :: Int
                                  }
                                  -> Effect
                                       { foo :: Int
                                       }
    render :: Effect ReactElement
    this :: ReactThis
              { label :: String
              , children :: Children
              }
              { a :: String
              , b :: Int
              }

in value declaration testChildren

With the inverse (componentDidUpdate without getSnapshotBeforeUpdate) inferring ReactUnusedSnapshot is fine, because it's equivalent to a Unit type.

I'm open to suggestions here.

[ethul]

All of the updates look great. Thanks for adding context. Resolves #64 . I like your proposal for getSnapshotBeforeUpdate and componentDidUpdate, but I will have to put a bit more thought into this.

[natefaubion]

I've gone ahead and pushed that approach. I've just consolidated it into a separate class so the signature doesn't look too bad.

[natefaubion]

Also, I should point out I haven't tested this thoroughly beyond type checking/inference 😆

[ethul]

Thanks! And I will give it a whirl this weekend or Monday.

On Sat, Jun 2, 2018 at 22:57 Nathan Faubion notifications@github.com wrote:

Also, I should point out I haven't tested this thoroughly beyond type checking/inference 😆

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/purescript-contrib/purescript-react/pull/149#issuecomment-394131331, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVYy9DFCvuhnVr_PIMUs2BnXLqs3Q0yks5t41CVgaJpZM4UV7UO .

[paf31]

Something seems off, since I can set render: 42 in my spec, and things compile happily:

R.pureComponent "bad" \this -> 
    pure { state: { }
         , render: 42
         , shouldComponentUpdate: \_ _ -> pure true
         } 

[natefaubion]

@paf31 Thanks! I had introduced a bug when I refactored it to use Nub. It was done in such a way that the provided state and render were removed from the spec before unifying. Also, pureComponent should not allow shouldComponentUpdate since that's the whole reason it exists.

[natefaubion]

I've added unsafe variations of the createElement functions which do not use ReactPropFields.

[ethul]

Thanks! Is this PR about ready to go? If so, I will give the changes a test and merge.

[natefaubion]

I don't think I have anything else to add to this PR.

[ethul]

Looks great and works well! Thanks for all of your work on this.