liqula / react-hs

A GHCJS binding to React based on the Flux design. The flux design pushes state and complicated logic out of the view, allowing the rendering functions and event handlers to be pure Haskell functions.
32 stars 10 forks source link

registerInitialStore should handle wrong usage better. #9

Open fisx opened 7 years ago

fisx commented 7 years ago

registerInitialStore should throw an error when called again, or it should clear the state, and it should be made clear that all code will be aware of the new state at that time.

Instead, the documentation disallows initializing a store type twice, but the function does not throw an error if we do that.

tysonzero commented 7 years ago

What is the reason for forcing there to only be one instance of each type of store? It seems like this goes very much against the whole "mutable global things aren't so great" thing, that most people seem to agree on. Why not have registerInitialStore return an actual object (like old mkStore), and pass those around. That way we can create multiple stores of the same type, and they wouldn't be global.

fisx commented 7 years ago

The old mkStore returned an MVar that was created inside an unsafePerformIO, so arguably the new situation has improved on that a little.

Passing the pure store value around would require significant refactoring of the way flux/redux actions are processed. The current design pulls a global MVar and updates the content.

As a work-around if you want to have to store values of the same type, you can wrap one or both of them in newtypes.

I agree that there are probably better designs, but I'm not sure what they would look like (yet), hence this mini-issue that's just about handling wrong uses of the library better.

Does this make sense to you @tysonzero? Or are you suggesting something I've missed?

tysonzero commented 7 years ago

Yeah I definitely agree it is better than the old mkStore method, but IIRC there was a period where the unsafePerformIO was removed from mkStore and it instead returned an IO ReactStore or something like that, which I think is what I am looking for, but that could have also not given me the results I wanted, not 100% sure.

And yeah I was talking about passing around a pure reference to the mutable (impure) store. Basically I want the semantics of newIORef, like pretty much exactly.

Newtypes definitely help, one use-case I was thinking about was having mutable stores for each child in some sort of collection. Not sure if that is even the right way to model collections of objects, but if it is then I would need something beyond newtype.

I do of course support fixing this issue, just seemed like a decent place to ask about this.

Also side note, it seems like mkStatefulView and registerInitialStore both set the initial value, which should I use, or both?

fisx commented 7 years ago

Let me start with your last question. (I hope I will get the react / flux terminology right, I am also a bit new to this.)

local state is something that only the component has access to, it is initialized during rendering, but can be changed between re-renderings by events triggered and handled inside the component.

stores are the flux part of this code base. if it was redux, not flux, there would be only one, all actions would be sent to that store, and all components created with mkComtrollerView would get a copy of it. flux can be used to emulate redux, but it is more flexible (for better or worse): you can have as many stores as you want, and pass any number of those to any of your controller views. but you don't have to have a 1:1 correspondence between views and stores.

stores are necessarily global, because they are accessed both by action handlers and by controller views, and i doubt it is possible, let alone feasible, to pass around pure values between all handlers and views so that everybody always gets the latest copy, and knows where to pass it along to.

that's why it's ok find to do this:

{-# NOINLINE globalStoreMap #-}
globalStoreMap :: MVar (Map TypeRep (Dynamic a)
globalStoreMap = unsafePerformIO $ newMVar mempty

now you can use globalStoreMap in any action handler or in any controller view. this is ok because the unsafePerformIO is only triggered once, and you can reason that it will be triggered soon enough for the mvar to exist once we need it. note the NOINLINE is crucial, or you would get different mvars in different calls to globalStoreMap.

you can read up on concurrent programming in haskell for a deeper discussion on how to avoid race conditions and dead locks, but the thing is you do have a concurrent programming task here, and you can't solve it with a program that is one giant pure function.

now that i'm reading your first two paragraphs again, i wonder if i misunderstood you. you want newIORef, not a pure function. but then what do you want to pass around as a pure value? the IORef? where do you want to dereference it?

note that MVar is thread safe, whereas IORef is not.

I think it's best to start your application with one global store (like in redux) that you dereference and pass as a pure value in all your controller views, and from then on it's all pure computation (until an action is triggered, and then it starts anew). then, if you find a reason to create a new global store, you have the option with this library. i'm not sure yet that there ever will be a reason for me, though. i think performance may be one, but i'm skeptical. lenses are a pretty convenient and efficient way to take large types apart into smaller ones to pass down to your sub-components.

hum. we should put this stuff into the documentation somewhere.

tysonzero commented 7 years ago

Ok I think that all makes sense. I guess my plan was to create instances of the stores in main or something like that, and then pass them explicitly throughout my views and such.

As for the MVar vs IORef stuff that was just an example, MVar is fine. I was just thinking that we would have something like store :: IO (MVar MyStore) without any unsafePerformIO, and use main to pull out and propagate the MVar MyStore itself.

So main and perhaps later on some other IO functions would convert IO (MVar MyStore) to MVar MyStore, and then the renderer that gets called every time something changes would covert MVar MyStore to MyStore.

fisx commented 7 years ago

I was just thinking that we would have something like store :: IO (MVar MyStore) without any unsafePerformIO, and use main to pull out and propagate the MVar MyStore itself.

that does not work very well. it would force you to pass along the MVar as an extra argument to every function that (directly or in some other function it calls) fires some action. i've tried similar things and come to the conclusion that it's must better to have a global call to unsafePerformIO.

tysonzero commented 7 years ago

I will admit I have not dove deep enough into such a topic to really argue on that one, so for now I'll take your word for it. In that case I just wish Haskell had an official way to support such a setup without unsafePerformIO, maybe a better module system so you could do a local import where you actually pass that MVar into the module itself when you import it, then every module will have top level access to any and all MVars necessary. So:

main :: IO ()
main = do
    foo <- mkStore (Foo [1, 2, 3])
    import qualified Module foo as M
    ...