purescript-react / purescript-react-basic-hooks

An implementation of React hooks on top of purescript-react-basic
https://pursuit.purescript.org/packages/purescript-react-basic-hooks/
Apache License 2.0
200 stars 33 forks source link

unsafePerformEffect component #41

Open jamesdbrock opened 4 years ago

jamesdbrock commented 4 years ago

Your initial "actual" example is a great way to write it -- keeping the PS bits effectful and running unsafePerformEffect once at the top level where next.js can pick it up. A component library could similarly expose both the effectful version (which might support a generic type constrained with type classes) alongside a fully applied version which chooses a type for some common use case.

Btw, this is explained briefly in the docs, but if that was confusing or not enough info let me know.

Originally posted by @spicydonuts in https://github.com/spicydonuts/purescript-react-basic-hooks/issues/12#issuecomment-573794368

megamaddu commented 4 years ago

hmmm?

jamesdbrock commented 4 years ago

...continued...

I suggest that the documentation for component should specifically encourage users to call it in unsafePerformEffect at the top level.

No-one will believe that calling unsafePerformEffect is the right thing to do unless instructed that way. I was very confused about this until I found this comment in this issue.

megamaddu commented 4 years ago

I wouldn't say it's the right thing to do. I prefer the "app bootstrap effect" pattern that's been used in examples (which I've unfortunately broken, but you can switch the git tag to the last major version to find them).

Definitely need a lot of documentation work, I'll leave this open. But yeah, I recommend using main to effectfully construct your root component, and construct the components each child component depends on there in that child's own component creation effect. This is what the Component props alias is for.

megamaddu commented 4 years ago

For example:

main = do
  app <- mkApp
  render (app unit)

mkApp = do
  ctx <- createContext ...defaultCtxValue
  child <- mkChild ctx
  component "App" \_ -> React.do
    ...
    pure $ provider ctx currentCtxValue [ ...more stuff ]

mkChild ctx = do
  component "App" \_ -> React.do
    xyz <- useContext ctx
    pure ...
pete-murphy commented 3 years ago

I was recently trying to use useContext in a purescript-cookbook recipe to implement a react-router-style Link component. The Link might be used in any leaf node of the app, so I was initially using unsafePerformEffect to avoid threading the context through. After reading this thread, I thought better of it and refactored to use a ReaderT context wrapper around Component, but now I'm thinking that's not really worth the overhead and I should just thread the context through explicitly as you recommend 😅 .

I'm curious though, since I've only used the library for small examples, what approach you would recommend if you find yourself needing a context at many leaf nodes of a large app. Would you end up using unsafePerformEffect? Or does the ReaderT approach make sense? Or is the situation just an anti-pattern that should be avoided? (It seems like wire-react-router avoids using Context, but I haven't yet figured out how that library works.)

megamaddu commented 3 years ago

We use unsafePerformEffect, partly because it matches the JS convention and we transitioned from a JS app initially, but also because it can be more convenient with lots of leaves. But it does occasionally bite us with unexpected component remounts. That's just a cost you have to manage if you go that route, just like you would in a JS React app.

Your ReaderT example is probably the best approach though, if you're starting from scratch. Maybe the react-basic-hook Component type should be type Component props = ReaderT (RouterContext) Effect (props -> JSX) to encourage that pattern. Or at least expose a ComponentT of that type. You'd have to redefine it with explicit types in your app anyway.