-
Notifications
You must be signed in to change notification settings - Fork 19
Feedback Requested - HyperView State #180
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
Conversation
db48033 to
b7b1a30
Compare
|
I can take a look during the weekend if you'd like. |
adithyaov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not throughly reviewed the inner wiring but from a user perspective, I like this a lot!
I don't have enough experience with this to see the limitations though.
Apologies for such a delayed feedback.
| data Counter = Counter | ||
| deriving (Generic) | ||
| instance ViewId Counter where | ||
| type ViewState Counter = Int | ||
|
|
||
| instance HyperView Counter es where | ||
| data Action Counter | ||
| = Increment | ||
| | Decrement | ||
| deriving (Generic, ViewAction) | ||
|
|
||
| update Increment = do | ||
| modify @Int (+ 1) | ||
| pure viewCount | ||
| update Decrement = do | ||
| modify @Int (subtract 1) | ||
| pure viewCount | ||
|
|
||
| viewCount :: View Counter () | ||
| viewCount = row $ do | ||
| n <- viewState | ||
| col ~ gap 10 $ do | ||
| el ~ dataFeature $ text $ pack $ show n | ||
| row ~ gap 10 $ do | ||
| button Decrement "Decrement" ~ Style.btn | ||
| button Increment "Increment" ~ Style.btn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very neat. Are there any limitations you see with this approach? (limitations in terms of what is possible to represent)
Alternatively, any consequences?
As I understand it,
The HyperView modifies the state and the view is derived from the state.
It looks like this also changes some idioms. This is no longer htmx way. It's closer to elm like model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed you mention a few limitations in the opening comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this also changes some idioms. This is no longer htmx way. It's closer to elm like model.
@adithyaov I would say that is the main downside to this PR. I really value how intuitive and simple the HTMX / current Hyperbole model is. I predict that half of our users will use state for everything, whether or not it is the best way to solve a problem. New users might be confused to see different sets of examples.
It IS completely opt-in: you can completely ignore this feature and develop as normal, but we shouldn't underestimate how it increases complexity.
I'm on the fence. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually misunderstood the usage,
What I think will be a nice UX:
update Increment prevState = prevState + 1
update Decrement prevState = prevState - 1In this PR it is currently:
update Increment = do
modify @Int (+ 1)
pure viewCount
update Decrement = do
modify @Int (subtract 1)
pure viewCountThis is actually a little confusing because of the fact that it supports both types of updates - The htmx way and the elm way.
The bias is towards HTMX-like pattern because of the return type. It's harder to reason about as potentially both the state and the return html can be modified.
Would it be possible keep the older HyperView class as is and introduce a new typeclass ElmView that only supports elm like pattern?
I really like the pattern of managing state. But it would be best if we are able to introduce it without modifying how the current HyperView looks like. Or we modify the current HyperView completely.
I'm happy to spend some time on experimenting with this if you think ElmView is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking mostly about the UX. We can also possibly create type aliases for this type of UX.
HyperView = HyperViewInternal () View
ElmView s = HyperViewInternal s s
I'm thinking out loud - this may not even make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you expect a reducer when Haskell already has a State monad/effect?
My reasoning is more on the lines of constraining the usage.
The update has 2 moving parts to it.
- The return html
- The state
HTMX: Modify html while keeping the state constant
ELM: Modify state while keeping the html constant
??: Modify both html and state
I want ?? to be undescribable.
I'm happy with using state if we keep the return type of update () instead of View ().
Thanks so much for the feedback by the way! It's super helpful to discuss it with you
Glad to be of help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMX: Modify html while keeping the state constant
Who says it keeps state constant? HTMX and Hyperbole both encourage the user to modify state in an update. It's just that currently state is tracked in a database, query parameters, sessions, MVar, or passed to actions. We have a whole section on the examples site dedicated to different ways of modifying state: https://hyperbole.live/state/actions
This PR adds a built-in view scoped State effect to the update monad, and a static Reader-like effect to the view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe ?? is okay.
All this PR does is add a build-in state effect to update and view such that one can easily track view state.
Going over it again - it may not be as hard to reason as I think. The implementation is very clean.
Maybe I'll sleep over it and let you know tomorrow. It may not be as big of a deal as I'm assuming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the pushback! Give it some more thought, maybe play with the branch, and let me know. I am definitely noticing from our conversation that it isn't intuitive for someone who is familiar with the framework!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that currently state is tracked in a database, query parameters, sessions, MVar, or passed to actions.
You're right. But for some reason, I've got into a habit of thinking of a frontend-state and a backend-state.
And in this context, state for me is always the frontend-state - Eg. state passed to actions/sessions/params (ephemeral state?).
That maybe incorrect way to think though. It's a habit derived from using front-end frameworks.
From our conversation, I'm more accepting of changes in this PR. I'll play around with this branch and let you know.
Looks like state passed to actions is exactly what we are doing now but making this state a first class citizen for the HyperView.
b7b1a30 to
6ef11f5
Compare
|
@seanhess, Is this change (semantically and functionally to an extent) exactly the same as threading state across actions? |
Yes exactly. It's a different interface, but it behaves the same way. The framework just handles the threading for you. It would be useful any time you find yourself adding a state parameter to every single action, like with the threaded counter: |
|
This is a very welcome change :-). I have a much more clear idea of what's going on now. The only thing I would point out again is that the framework now supports the elm-like idiom. Although, I don't think this is a bad thing. I would prefer the solution in this PR for state management over threading it in actions. It's actually great that this is a rather non-invasive change, both in terms of functionality and the usage.
I'm not immediately able to see this. |
Well, the HTMX model of: On the other hand, this isn't nearly as big of a complexity increase as Elm. I like how Evan taught people to arcitect their apps well, I disliked how he treated us as children and removed all the footguns. I think I'm ok with a middle ground, and this PR is probably that :) Having component state is one of the most requested features. Let's do it. I think I'm convinced. |
6ef11f5 to
3fe2472
Compare
3fe2472 to
bac69a8
Compare
conflict
bac69a8 to
4152e14
Compare
I added optional view state to HyperViews. I'd appreciate some feedback from anyone interested.
ViewState is entirely backwards-compatible: the default is
()In the name of simplicity, I've resisted implementing this for a long time. We aren't trying to implement the Elm Achitecture. Instead we have a simpler model that follows the design philosophy of htmx: actions call the server and replace the view with something. However, I don't think it will hurt to have it as an option.
Some outstanding limitations:
modify,get,put, etc are fromEffectful.State.Static.Local- It would be useful to export them from somewhere, but they might conflict with otherStatefunctions, likeEffectful.State.Dynamic. They also require type annotations. Should we have specialized versions?ViewIdinstead ofHyperViewdirectly. (It makes working with dumb Views a pain in the ass if they all have to be members of HyperView).