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

Rename `someStoreAction` to something shorter #30

Closed tysonzero closed 7 years ago

tysonzero commented 7 years ago

I end up basically having to have one line per action due to the combination of the long function name and the type application plus the parameters themselves. IMO a name like action would be great.

fisx commented 7 years ago

Look at the TODO example: someStoreAction is only used once in the dispatch function, and the handlers use that.

This is off the top of my head, hope I got it right. Please re-open if not.

tysonzero commented 7 years ago

Yeah I see that, but dispatchTodo is a function that basically does nothing. All it does is redirect to someStoreAction, I mean it is literally just dispatchTodo a = [someStoreAction @TodoState a].

It seems silly that in order to have short code for actions you have to have a module with just a single function that is shorthand for an existing library function.

I would still very strongly consider renaming it to action, and I would also consider changing the example to remove the dispatch module, it seems pretty useless.

fisx commented 7 years ago

I don't understand your point.

  1. you don't need to make Dispatch a separate module. just add the shortcut anywhere you like.
  2. having an app-specific shortcut makes the call shorter by the type allication (which cannot be guessed by the library), so the burden of writing a one-line function in your app gives you many shorter lines in the rest of your code.
tysonzero commented 7 years ago

I mean there isn't much difference in length between:

action @TodoStore

and:

dispatchTodo

And the latter requires defining it in some module and importing it everywhere.

I also still think it might be worth considering having the type family be required to be injective, I'm still not compelled by the code-reuse through a shared action type example.

fisx commented 7 years ago

Ok, renaming seems to be important to you, so let's:

I personally think action is too short, but your opinion seems stronger than mine. :-)

So if you PR me, I will merge.

... the type family be required to be injective...

You mean StoreAction, correct? That would mean that if you want to trigger a transform in two stores, you'd have to dispatch two actions instead of one, one for each store type. I don't have a strong opinion on this. What is the problem you want to solve with this? If there is one, you can add this change to the same PR.

tysonzero commented 7 years ago

https://github.com/liqula/react-hs/pull/35

tysonzero commented 7 years ago

You mean StoreAction, correct? That would mean that if you want to trigger a transform in two stores, you'd have to dispatch two actions instead of one, one for each store type. I don't have a strong opinion on this. What is the problem you want to solve with this? If there is one, you can add this change to the same PR.

So I am trying to avoid the bloat of all the type applications, I still haven't found a proper practical use for actually having two stores share the same action type. I remember there being one example somewhere in the codebase but I wasn't really convinced. Particularly since you can always use a newtype to mimic the same behavior.

So basically just reducing code bloat. In general most of my changes are just aimed at ergonomics and flexibility, I really want the library to be more usable and concise than react.js.

fisx commented 7 years ago

ok, makes sense.

so, we just say type StoreAction s :: a | a -> s?

tysonzero commented 7 years ago

Precisely!

fisx commented 7 years ago

so, we just say type StoreAction s :: a | a -> s?

Precisely!

Works for me. @divipp, any objections?

fisx commented 7 years ago

I just checked, and the syntax that actually worked is this:

    type StoreAction (storeData :: *) = action | action -> storeData

Alas, this is violated in TestClient.hs. Yes, you could just use newtype and yield more actions to resolve this...

divipp commented 7 years ago

I have no objection.

fisx commented 7 years ago

Ok, thanks. I started wondering though how essential the concept of sending actions to more than one store is to flux.

@tysonzero how do you feel about postponing this and figuring out how to separate react and flux first? Wouldn't that make more sense than watering down flux into something not flux and not redux?

tysonzero commented 7 years ago

Yeah I'm more than happy to prioritize that first. Is the idea of one action being usable on multiple stores pretty fundamental to flux?

fisx commented 7 years ago

not an expert, but yes, i think so. evidence for it is that the changes to the example would be a bit awkward already.

fisx commented 7 years ago

I think everything in here but the discussion on #20 has been resolved.

Please re-open, or better yet, open a new issue if I have overlooked anything. Thanks!