easafe / purescript-flame

Fast & simple framework for building web applications
https://flame.asafe.dev
MIT License
292 stars 10 forks source link

Suggestion: Maybe type for event handlers #26

Closed chekoopa closed 3 years ago

chekoopa commented 4 years ago

Now handleRawEvent under Flame.Renderer.toVNode just calls handler and gives its result to updater function, so when you're writing event handlers you have to return message regardless of the event's content.

For example, we would want to have a conditional event, like "on Enter key pressed", which now is implemented like this:

{- ... -}
import Flame.HTML.Attribute (createRawEvent)
import Web.Event.Event as Event

-- | Tries to extract a keycode from the event.
keyCode :: Event -> Maybe Int

onEnterPressed :: ∀ msg. msg -> msg -> NodeData msg
onEnterPressed onFail onEnter = HA.createRawEvent "keyup" $ \event -> do
  Event.preventDefault event
  case keyCode event of
    Just 13 -> pure $ onEnter
    _ -> pure $ onFail  -- notice we can't just suppress it

The only possible alternative is just using onKeyup and filtering the pressed key in update function, but it mostly adds bloat in the application's logic (if you need to catch only one particular key). One could try throwing an error to break the event handling, but it also breaks toVNode execution, too. Yes, we could just wrap handler into try, but it either prevents us from handling real errors or forces unpleasant trace messages in the console.

Therefore, the proposal is to change event handler's type to Event -> Effect (Maybe msg), so we could filter the event right in the handler. Also, it would look more consistent considering how EffectList update function deals with events (using Aff (Maybe msg)) and channel message types (either Array msg or Maybe msg).

The updated example code would look like this:

onEnterPressed :: ∀ msg. msg -> NodeData msg
onEnterPressed onEnter = HA.createFilteredEvent "keyup" $ \event -> do
  Event.preventDefault event
  case keyCode event of
    Just 13 -> pure $ Just onEnter
    _ -> pure $ Nothing

The proposed changes wouldn't break existing API, as already placed event constructors only get an additional Just wrapper under the hood. But additionally, we'd get a createRawEvent' or createFilteredEvent (the naming is a point of discussion).

easafe commented 4 years ago

Hello @chekoopa

I am not against this, but I can see, for example, you finding out later that you need to handle more than just enter, or writing a bunch of onXPressed functions.

That being said, I don't see any issues either with changing RawEvent to return Maybe Message (and consequently HE.createRawEvent) instead of adding yet another create*Event function.

Do you think you can provide a PR for this? Btw, what sort of things are you using flame for?

chekoopa commented 3 years ago

Hi again :relaxed:

I can see, for example, you finding out later that you need to handle more than just enter, or writing a bunch of onXPressed functions.

Sure thing, in case when you need more than one-two buttons, you put a onKeyup and process it in update. I'm talking about an amount of little cases such as submit-text-on-Enter or mouse dragging (I'm now working Elm's DnD library port, because it turns out we need reorderable lists :laughing:)

That being said, I don't see any issues either with changing RawEvent to return Maybe Message (and consequently HE.createRawEvent) instead of adding yet another create*Event function.

Yes, I've thought about that it the first place, however it's an API-breaking change. If you're not against that (bumping a major version number), I can do. And as a code needed to implement this is pretty little, all I need is just to sit and code it.

Btw, what sort of things are you using flame for?

Flame is now becoming a part of our company's stack (besides of Haskell, NixOS and a little of Python) as a main frontend tool (actually, it's the third stop as for frontend tools, after Elm and Hedwig). We develop the educational interactive stand about intelligent energy systems, btw.

easafe commented 3 years ago

Yes, I've thought about that it the first place, however it's an API-breaking change. If you're not against that (bumping a major version number), I can do.

Yea, thats fine. I don't think anyone depends on that, anyway.

Flame is now becoming a part of our company's stack (besides of Haskell, NixOS and a little of Python) as a main frontend tool (actually, it's the third stop as for frontend tools, after Elm and Hedwig). We develop the educational interactive stand about intelligent energy systems, btw.

Interesting!