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

Decoupling React from Flux #31

Open tysonzero opened 7 years ago

tysonzero commented 7 years ago

As mentioned earlier I think it would cool to have this be the defacto react implementation for Haskell, and in order to do that it would really help to be state paradigm agnostic. Either by having a separate react-flux that depends on this package that does all the flux stuff, or by at least moving the flux stuff into a React.Flux module. I also think that this decoupling would have a side effect of cleaning up the interface.

So I guess probably the first thing that should be looked at is all the event handlers, they currently require either [SomeStoreAction] or state -> ([SomeStoreAction], Maybe state). My thinking is that since store actions can already perform completely arbitrary IO, then allowing IO in the event handler is no more dangerous, but I could be missing something. So I would make their types be IO () and state -> IO state.

In the long run I would also suggest getting rid of the state -> IO state one, and would make the stateful view stuff be in its own module as another state paradigm (that you can mix and match of course), and I would have it so that the mkStatefulView lambda has an extra argument that contains a reference of sorts to the view, and then in the IO () block you can call setState theRef newState and also oldState <- getState theRef. This way you can edit states of parent views and aren't just limited to the one you are in.

Another side benefit of this stuff is we can finally have preventDefault and such not be unsafePerformIO plus seq shenanigans.

Perhaps the above indicates that maybe we would actually want our own monad / monad transformers, as it is nice to have things not be just in IO. Although there fundamentally is a lot of IO going on, so that will probably come quite a bit later if it does happen.

fisx commented 7 years ago

Thanks for your thoughts! As I said in https://github.com/liqula/react-hs/issues/1#issuecomment-308013654, this does not have a high priority for me, but I am open to making the design more modular.

[Refactor module structure to separate react and flux]

I think what we need before we can start with this is a concrete (as opposed to hypothetical) alternative to flux. If we have that, it will be much easier to see what belongs where. Without that, it's not only irrelevant, but also much harder, to do this refactoring. (We have an alternative in mind which is based on https://wiki.haskell.org/LGtk/Semantics, https://hackage.haskell.org/package/lensref, but that is not fully implemented yet.)

[IO for handler types]

Handler types are pure by design, and I feel very strongly about this being a good idea. It's a good think that you only have one place where effects can happen.

[move StatefulViewHandler away from the library core]

@divipp made some tentative changes yesterday in #28 (see the last commit) that conflict with this idea. I'd be interested to hear whether you like those changes.

[preventDefault and friends]

Check out https://github.com/liqula/react-hs/pull/28/commits/5fb3388f9322dcc48ad2506ea37d52db9216ad45 (same PR). I'm not completely happy with that, as it requires to add simpleHandler to all your handlers even if you don't want to do anything, but I suspect something very much like this could be done with a custom monad that instantiates MonadWriter [EventModification] (rather than using IO). But this is so far the only effect that I can see happening in the event handler, before the action is processed in the transform.

I guess my biggest question for now is: what do you want to use instead of flux? (And I think redux doesn't count, as it's just a restricted version of flux, and we can easily emulate that already.)

tysonzero commented 7 years ago

I think what we need before we can start with this is a concrete (as opposed to hypothetical) alternative to flux.

So fundamentally what I am looking for is genuinely reusable components. Which is pretty difficult with flux architecture since fundamentally everything with interesting behavior needs to know about the global and difficult to duplicate set of stores.

The state architecture I would like to use is the one used just in react-js, which is having the views have state, and then they can pass down either direct references to the state or functions that interact with said state in a controlled way. This is the way I did it when I wrote my app in react.js, honestly I basically just wanted to mimic the same general structure in Haskell.

So I guess I would like to have mkStatefulView have type:

JSString -> state -> (Ref state -> ...) -> View ...

So basically you pass in the initial state, and a lambda that takes in a reference to that state, and then in the relevant places (event handlers basically), you will be able to interact with that state. This can do everything the existing method can do, but it also has the huge advantage of allowing child views to interact with their parent's state (either directly, or by passing in functions that interact with it).

One example way I used this was I add a login view with a child that depended on the URL (so it was basically a "layer" more than a view), and it had the user token and user information as well as login, logout and such functions all encapsulated within it, and it would pass to its child read only access to the user data, as well as a getHeaders function that added the user token header, as well as the login / logout functions.

Another example was I had a search results page with the search results in its state, it had a child view for the search bar that it gave a updateSearch function to, that allowed the search bar to call whenever it wanted to update the search, it then also passed down its state into a resultList view that just displayed all the data line by line but didn't mutate anything.

It's also nice for dealing with forms, currently I have to put form errors in a global store, since I need IO to see if the form will succeed or not, and once I am in the IO section I have left the safety of my stateful view and into the flux action. I would much prefer to have the state of the form fields and any form errors all stored within the state of that forms view.

Even if you want to use flux architecture, there is no real harm in making the stateful views strictly more powerful.

Handler types are pure by design, and I feel very strongly about this being a good idea. It's a good think that you only have one place where effects can happen.

But they aren't morally pure at all, since they can call an arbitrary action on an arbitrary store, which itself does arbitrary IO. If I wanted to do arbitrary IO in a handler, I can do it just fine right now, all I have to do is jump through hoops by creating a new store and new actions, so handlers aren't pure.

If there is a more technical reason for forcing IO to be in an action which can be called arbitrarily by a form handler, such as some sort of thread safety of the IO or state or whatever, then I will gladly take that into account. But IMO the current way does allow arbitrary IO, it's just inconvenient and makes it hard to use other state paradigms.

The reason why I think this needs to be changed, is that fundamentally the current type doesn't work for any state architectures besides flux, I mean it literally has a flux action in the return signature.

One option that might be the best of both worlds, is to have mkFluxView have the current type signature, and maybe a mkStatefulFluxView as well. Then have mkView that just has an IO () type for handlers, and mkStatefulView with that same type but that also has the whole Ref state thing. I think that really is the only way to decouple react from flux.

@divipp made some tentative changes yesterday in #28 (see the last commit) that conflict with this idea. I'd be interested to hear whether you like those changes.

Honestly not a huge fan of the change, its a class that doesn't have methods or laws, it doesn't help type inference or improve local reasoning or allow for extra functionality.

I guess my biggest question for now is: what do you want to use instead of flux? (And I think redux doesn't count, as it's just a restricted version of flux, and we can easily emulate that already.)

So yeah to summarize:

To allow for a different state paradigm we need to allow for event handlers of types besides [SomeStoreAction] and state -> ([SomeStoreAction], Maybe state, as they only work for the flux paradigm (even the state one is too limited for things like interacting with the state of a parent view). And in my view something of type IO () is fine, since SomeStoreAction is already morally IO () as the actions it calls can do completely unrestricted IO. To get the best of both worlds we could maybe havemkFluxView/mkStatefulFluxView` that have more restrictive handler types.

The alternative state paradigm I personally want to use is just the default paradigm-less react.js where all the views have state that they can pass down to their children, and they can also pass down functions for interacting with the state, (or in our case you could also just pass down the reference to the state directly). It allows for truly reusable components, and avoids global mutability which I personally find very enticing. We are close to that right now with mkStatefulView, but we need a true reference to the state in scope in the view definition, to pass down to child views.

tysonzero commented 7 years ago

I can see the benefits of flux for some parts of my application, particularly things like the current logged in user and other very top level data like that. For parts like that I can probably let go of my fear of global mutables. But in many situations I have found the need for floating things into top level global mutable stores extremely frustrating, I am sometimes even missing the pure javascript/react.js version, which isn't a feeling I should ever have.

I am really not finding much benefit in forcing a round trip through a bunch of actions in order to do the IO I want, I am honestly considering doing something like:

data IOStore = IOStore

data IOAction = IOAction (IO ())
    deriving (Generic, NFData)

instance StoreData IOStore where
    type StoreAction IOStore = IOAction
    transform IOStore (IOAction m) = IOStore <$ m

The fact that this is possible should show that SomeStoreAction is morally equivalent to IO (), but just in some cases much more annoying and verbose to deal with, requiring really annoying work arounds that complicate the code.

I have particularly found dealing with ajax forms frustrating. I just want the submit button to run the ajax request and either add in some errors to the local view state if the form post errors, or I want it to redirect to a url specified in the query-string on success, after updating some state. But the current setup makes that significantly harder and more verbose than it was in JavaScript, I might have to give each form its own global store just to hold the errors, and even with that I have to do a bunch of extra plumbing and round trip through a bunch of actions.

I would have preferred to do something like:

... = mkStatefulView ... $ \formRef -> do
    let submit event = do
        formData <- get formRef
        jsonAjax ... (getContent formData) $ \case ->
            Left error -> set formRef (setError formData error)
            Right data -> do
                doStuff data
                url <- get urlRef
                redirect (getNext url)
    form [onSubmit submit] $ do
        ...

But sadly I the closest I can get is trying to mimic the above by bouncing around actions filled with large amounts of data and updating a bunch of global state, even though its only relevant to the single form above.

Sorry if this sounds aggressive or rant-y at all, I'm just really frustrated and am having doubts about converting my application from native react.js.

fisx commented 7 years ago

I think the direction in which you are thinking makes sense. I have actually encountered a problem with local state that I managed to solve, but I'm not happy with the solution. Can you give us a little more time to do some more cleanup, and then start on a PR?

About effects in handlers: I accept that having to get access to effects is inconvenient and we should find a solution for that, but I disagree that the current implementation is morally equivalent to having effects locally in event handlers. With the same reasoning, you could argue that any pure function whose result value is used in an IO computation morally has type IO. In fact I actually like your IOStore hack (sorry :-). The benefit of having pure handlers is that you can easily reason about them and write pure tests for them. Nothing morally IO about that, no?

I would like all effects to be allowed in terms of constraint types and abstract m monads, not IO, but I think you've said you are not strongly opposed to this? This may also be a solution to your effects-in-handlers request, because you could simply add more constraints to the handler type. Can you give a code example where you need IO, and what for?

@divipp made some tentative changes yesterday in #28 (see the last commit) that conflict with this idea. I'd be interested to hear whether you like those changes. Honestly not a huge fan of the change, its a class that doesn't have methods or laws, it doesn't help type inference or improve local reasoning or allow for extra functionality.

note that the class has been removed in the commit i pointed you to; i introduced it earlier, but @divipp found a better way to do the same thing.

fisx commented 7 years ago

(I'm really sorry if this is frustrating for you. I'm trying hard to accommodate you, and I believe you make good points. We'll figure it out! :-)

tysonzero commented 7 years ago

I think the direction in which you are thinking makes sense. I have actually encountered a problem with local state that I managed to solve, but I'm not happy with the solution. Can you give us a little more time to do some more cleanup, and then start on a PR?

Yeah sounds great!

The benefit of having pure handlers is that you can easily reason about them and write pure tests for them. Nothing morally IO about that, no?

I'm confused by what you mean, [SomeStoreAction] isn't an instance of Show, Eq, or any other instance that could be useful for testing. So I really don't see what exactly you can test with [SomeStoreAction] that you can't with IO (). Both can be "tested" by simply not executing them and looking at the surrounding structure just fine.

I would like all effects to be allowed in terms of constraint types and abstract m monads, not IO, but I think you've said you are not strongly opposed to this? This may also be a solution to your effects-in-handlers request, because you could simply add more constraints to the handler type. Can you give a code example where you need IO, and what for?

If you look through my second comment you will see my jsonAjax example. That is the part that fundamentally needs IO. For my workflow to be nice (and somewhat state paradigm agnostic) I basically need to have the ability to have IO that interacts solely with the view state without having to bounce around through a bunch of stores and actions. I think having a state-ref (so probably some sort of TVar or MVar that runs in IO) of sorts along with IO in handlers is probably the best way to do this.

note that the class has been removed in the commit i pointed you to; i introduced it earlier, but @divipp found a better way to do the same thing.

I see some DataKinds stuff happening, although its a big commit, can you summarize what exactly was done? Now that I think about it wouldn't it be possible to just allow handler to be IO () and pretty seemlessly allow for a view that performs IO in handlers? Then we can have the current views be a part of the React.Flux module since they are fundamentally flux specific.

(I'm really sorry if this is frustrating for you. I'm trying hard to accommodate you, and I believe you make good points. We'll figure it out! :-)

Thanks! I know I'm asking for pretty big changes and I'm sure a lot of you are perfectly happy being tied in to Flux, so I appreciate the accommodation. And I do think that even Flux users will benefit in certain situations from these changes, since fundamentally mkStatefulView is fairly limited right now, and simple things like handling forms and having errors in forms without requiring an entire store for that form are currently impossible.

Out of interest, how do you currently handle forms that send ajax requests that might fail currently? To me it seems to straight up require a form error store for each and every form, which sounds like a real pain, and so far that has been one of my biggest pain points with the current architecture.

fisx commented 7 years ago

I'm confused by what you mean, [SomeStoreAction] isn't an instance of Show, Eq, or any other instance that could be useful for testing

good point. easy add those instances, but obviously i haven't done this yet.

If you look through my second comment you will see my jsonAjax example.

This is not very convincing. The intended use of state is to update it in a StatefulViewEventHandler. You are using a "never changing" state ref whose referenced value is mutated in IO. Ok, you need IO for that, but you're not supposed to do that.

25

Yes, there are a couple of changes. I'll see that description is updated later today.

Out of interest, how do you currently handle forms that send ajax requests that might fail currently? To me it seems to straight up require a form error store for each and every form, which sounds like a real pain, and so far that has been one of my biggest pain points with the current architecture.

I think you're right here.

Anyway. I will slow down now responding in this thread for a little, try to get #25 polished (and hope you'll like it once it's properly explained), and then we can get back to discussing this.

Thanks again for your input and patience!

tysonzero commented 7 years ago

good point. easy add those instances, but obviously i haven't done this yet.

Wait it is? But SomeStoreAction is an existential type that intentionally throws away all its type information, there is no possible way you could implement any meaningful Eq or Show, or am I missing something? Particularly since it can contain IO actions as I showed, or just regular old functions too.

This is not very convincing. The intended use of state is to update it in a StatefulViewEventHandler. You are using a "never changing" state ref whose referenced value is mutated in IO. Ok, you need IO for that, but you're not supposed to do that.

So wait how do you plan on accomplishing the simple and commonly done task of having a form spit out server side errors to the user on fail, would you have a whole dedicated error store for every form? I really don't want that, and I don't need it in react.js, so I would rather not have it here. This seems like an ideal use case for stateful views, but it really does require doing some IO without going off to the actions. To me it seems like the only two real options are having some way (perhaps optional) of allowing IO in handlers, or just sacrificing usability substantially when compared to react.js.

Anyway. I will slow down now responding in this thread for a little, try to get #25 polished (and hope you'll like it once it's properly explained), and then we can get back to discussing this.

Thanks again for your input and patience!

Sounds great! Thanks!

fisx commented 7 years ago

(those are both good points. obviously i'm not paying enough attention to this conversation, so as agreed, i'll be back in a bit.)

tysonzero commented 6 years ago

I was wondering if you had any further thoughts on this. It seems like the only truly framework agnostic way to handle views is to allow callbacks to do IO, which at the moment they can already do rather inconveniently via stores. As far as I can tell we don't lose anything by putting them in IO, since the current setup does not give us any extra reasoning or bug prevention or power.

Next StatefulViewEventHandler needs to be an alias for state -> IO (Maybe state) instead of state -> ([SomeStoreAction], Maybe state), which would be sufficient for writing forms that aren't coupled to some Store.

We could instead use a ref based implementation for stateful views. This is honestly probably a much better idea than the above, it would allow us to get of the beginner unfriendly EventHandlerType stuff altogether. This way we can pass in the ref to child views (directly or indirectly via closures) and call setState in any handler that has access to the ref, just like how it works in react.js.

At that point we have pretty much succeeded in decoupling react from flux! We would have React.Flux.View which would export mkControllerView, React.View which would export mkView and mkStatefulView, and stores would be in React.Flux.Store. The remaining modules would be split up appropriately.

Once that is done, if you don't import from React.Flux you will never touch actions or stores, and will be fully able to implement any state handling paradigm you want. I actually have an idea for a state paradigm that I'm hoping would give good performance while still being completely modular and reusable, so once these changes are done and I have some time I will probably play around with that.

tysonzero commented 6 years ago

@fisx I'm currently working on a pull request for this now. Once this is done I would personally be interested in improving documentation and then pushing to Hackage. Of course feel free to give feedback anytime.