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

unsafePerformEffect component #41

Open
jamesdbrock opened this issue Nov 4, 2020 · 6 comments
Open

unsafePerformEffect component #41

jamesdbrock opened this issue Nov 4, 2020 · 6 comments

Comments

@jamesdbrock
Copy link
Member

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 #12 (comment)

@megamaddu
Copy link
Member

hmmm?

@jamesdbrock
Copy link
Member Author

...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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

pete-murphy commented Feb 8, 2021

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
Copy link
Member

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.

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

No branches or pull requests

3 participants