Skip to content
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

Events Can Be Maps. So Too Subscription Queries #644

Open
mike-thompson-day8 opened this issue Sep 2, 2020 · 31 comments
Open

Events Can Be Maps. So Too Subscription Queries #644

mike-thompson-day8 opened this issue Sep 2, 2020 · 31 comments
Assignees

Comments

@mike-thompson-day8
Copy link
Contributor

mike-thompson-day8 commented Sep 2, 2020

re-frame will soon allow:

  • events to be maps.
  • queries to be maps.

Events

Assuming this ns:

(ns my-app.my-namespace 
  (:require [re-frame.core :as rf]))

You can still dispatch events which are vectors, as before:

(rf/dispatch [:some-event-id  arg1])

but, now, you can also dispatch a map:

(rf/dispatch {::rf/eid :some-event-id   :some-thing arg1])

Within the map being dispatched, notice the key :re-frame.core/eid via the aliased namespace. eid means event-id. The value of that key is the event identifier. The other keys in the map are application-specific.

Queries

You can still subscribe using a vector, as before:

(rf/subscribe [:some-query-id  arg1])

but, now, you can also subscribe using a map:

(rf/subscribe {::rf/qid :some-query-id   :a-name arg1])

Within the map supplied to subscribe, notice the key :re-frame.core/qid via the aliased namespace. The value of that key is the query-id. The other keys in the map are application-specific.

Writing Handlers

You write handlers for map-encoded events as you would other handlers, it is just that the event argument is a map not a vector:

(rf/reg-event-fx 
  :token 
  (fn [coeffects event]
    ... event is a map))

Open Questions

  • is :re-frame.core/eid a good choice for map key? Other?
  • is :re-frame.core/qid a good choice for map key? Other?

They should be namespaced, yes? Is eid too cryptic? Prefer a longer more explicit name?

I'm unhappy with ::rf/eid. That's a horrible mouthful for a newbie. Maybe the tutorials just teach the vectors way and avoid exposing newbies to event maps early on.

Timing

@superstructor will check-in a first cut (to master) within a day or so. But a release is probably a week away, We'll wait for feedback. And bring a couple of libraries and utilities along.

Likely Objections

Question: Won't this approach make dispatching and subscribing more verbose?
Answer: Yes, it will. If you find that offensive, you don't have to use it. You can keep doing exactly what you are doing now. It is just that being able to use maps more easily solves some problems and we wanted to unlock that flexibility.

Question: Why not just use this partial approach (dispatch [:event-id {:param1 arg1}]) . Ie. make events a 2-vector of [event id a-map]. That would be backward compatible.
Answer: We did consider it. But we felt that design would be lukewarm coffee - we felt it was right to go all-in on maps.

A Breaking Change?

We are "growing the API" by allowing events to be maps. So, in theory, this is not a breaking change.

However, that's cold comfort. If you do start to use these two new features in your application, you might find that
certain re-frame libraries and utilities might break because they expect vectors, and you are giving maps.

These libraries will have to be changed to accommodate the expanded API flexibility. So don't use maps in your application until all dependent libraries and utilities are updated.

We are aware that the following utilities/libraries will need to be changed:

  • re-frisk
  • re-frame-10x
  • re-frame-http-fx
  • re-frame-async-flow

Are we forgetting any?

Why?

Terse Version

For software systems, the arrow of time and the arrow of entropy are aligned. Changes inevitably happen.

When we use vectors in both these situations we are being "artificially placeful". The elements of the vector have names, but we are pretending they have indexes. That little white lie has some benefits (is terse and minimal) but it also makes the use of these structures a bit fragile WRT to change. Names (truth!) are more flexible and robust than indexes (a white lie).

The benefit of using names accumulate on two time scales:

  1. within a single execution of the app (as data moves around on the inside of your app)
  2. across the maintenance cycle of the app's codebase (as your app's code changes in response to user feedback)

Longer

Back in 2014, I wrestled with how to model events in re-frame. Should I choose to represent events with vectors or maps?

In the end, I choose vectors. If I remember correctly, that was partly because I saw Pedestal App
doing messaging with vectors and it was designed by VIPs (very important programmers). And partly because
I believe aesthetics/ergonomics matter a lot and, at the dispatch point, using a vector is minimal and neat.
I was aware I was giving up something flexibility-wise, but it didn't feel like too much. Tradeoffs.

Mike, did you just try to shift blame to the Pedestal App team?
Oh. It was that obvious, was it? Look, I was a young, foolish and impressionable functional programmer led astray.

Now, in the intervening years, I got wiser slowly. I have become aware that some "falsely positional"
arrangements - arguments to functions, for example - are inherently more fragile WRT to "evolution" (aka the passage of time).

But I thought I had got away with it. I thought that I had managed to get all the benefits of more minimal aesthetics,
without the downside of fragility. Until last week.

I was in a design session with a colleague, Denis, and we were kicking about ideas when I was suddenly struck a bit speechless because I realized that the placefulness of vector events had completely stopped me from considering various design options. If I allowed myself to think about the problem at hand with events being maps, the design fell out kinda easily because that better allowed for evolution (of data over time, within a running app - which was one of the two time scales I identified above).

I hadn't got away with it after all, and this issue, which became all too easy via maps, has been an irritating
pebble in my shoe for waaaaay too long. I'd had blinkers on. And I'd been trying to work around the fake placefulness of using vectors for this purpose.

So, here we are. I'm out of that cage. Change is afoot.

Final thought: vectors really have served us pretty well. There'a only a couple of places where the additional flexibility of maps is going to be useful, but they are important ones.

Weak Example

The following example is a little weak, but I want to keep things simple. Sometimes, you need to
do some very CPU intensive work. Something which will tie up the single browser thread available to you.
You need to update the DOM (show the twirly?) and then hog that thread doing that CPU intensive work.

So, this is a two step process. Update app-db to show the twirly, and then wait for the next animation frame
to have passed so the UI is drawn. And only then start hogging the CPU. If you hog the CPU before the animation
frame, the twirly never gets shown.

If you have :flush-dom metadata on an event you dispatch, re-frame will wait until after the next
animation frame before handling it.

Okay, so that's the setup. We're going to need a two step process.

But of course, this is all implementation detail. Somewhere within an app, a button will dispatch
an event which says "do calc" but that view knows nothing about the two steps.

Let's model the event emitted by the button click as a map; (dispatch {::rf/eid :do-calc})

The handler looks like this:

(rf/reg-event-fx
 :do-calc
 (fn [{:keys [db]} {:keys [flushed?] :or {flushed? false}  :as event}]      
   (if-not flushed?

      ;; step 1 - 
     {:db        (assoc db :twirly true)
      :dispatch  ^:flush-dom (assoc event :flushed? true)}  ;; <-- adding to the map

      ;; step 2 - hog the CPU
     {:db (do-some-cpu-intensive-task db)})))

Notice that the event changes over time, in two steps. And that ability to change makes the design an easy one.

And, yes, this is a weak example because it simple. It could be achieved via vector events fairly easily. Maps come into their own with other, more complicated async situations like HTTP operations. More on all that soon, once we get this stage done.

@thheller
Copy link
Contributor

thheller commented Sep 2, 2020

I've been thinking about similar things recently regarding my own experimental react-less framework and I'm somewhat at the same point you were at years ago. Currently I'm also choosing the "prettier" approach of vectors but actually would prefer maps and will likely move to them.

As far as re-frame is concerned though I think the convenience issue pretty much solves itself if you introduce a second arity to dispatch (and probably subscribe).

(defn dispatch
  ([event-id]
   (dispatch event-id {}))
  ([event-id args]
   {:pre [(keyword? event-id)
          (map? args)]}
   (do-something-useful (assoc args ::event-id event-id))))

(dispatch ::some-event!)
(dispatch ::some-event! {:with "args"})

Probably with some extra compatibility when event-id is a vector to support the old style. Or maybe also allow a single arity map with ::event-id assumed present. Maybe that solves the ::rf/eid issue as the user never needs to actually type it out and you eliminate potential typos/invalid keys easily.

@niquola
Copy link

niquola commented Sep 2, 2020

Hi, we use same convention for dispatch:

(dispatch ::event-key {...})

;;or in simple case

(dispatch ::just-key)

internally dispatch will assoc event key into args map

Or even go father and:

(dispatch event-fn {...}) 

;; or to use defn meta

(dispatch #'event-fn {})

So compiler will check handler exists end skip exra handlers registry

@davidpham87
Copy link

I personally have some little concern of changing the type of the events, but I guess the world evolves! I hope it will not create many breaking changes. To solve the problem I was using

(reg-effect-fx
  ::some-event
  (fn [{db :db} [_ {:keys [args]}]
  {:db db}))

Same for subscriptions.

@p-himik
Copy link
Contributor

p-himik commented Sep 2, 2020

Just to expand a bit on what thheller has already said, because right now I'm in favor of that approach. In that case the context could look something like {..., :event {:id :event-a, :args {...}}}.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 2, 2020

@thheller
Oh yeah, that's a good idea, thanks!! I'll sleep on it, but I'm pretty sure we'll adopt that. And I can avoid using that overly terse key ::rf/eid - it can now be the more descriptive ::rf/event-id because it doesn't have to be explicitly used everywhere. That was really bugging me.

@p-himik
I don't see there being an :args key within the event map. That would create too much nesting which would then have to be destructured away in handlers. The event key will just be assoc-ed into args map, and it is that map which is the event. Many newbies probably won't even realise. And that's a good thing. Also, within context the :event is nested inside of :coeffect - did you mean to move it out?.

So (dispatch :event-a {:blah 5 :foo "hello"})

would result in a context of
{..., :coeffect {..., :event {:re-frame.core/event-id :event-a :blah 5 :foo "hello"}}}.

@p-himik
Copy link
Contributor

p-himik commented Sep 3, 2020

@mike-thompson-day8 Ah, sorry - I should've been more specific. My intention was to not pass event ID to the handler at all. I've never seen a need to know event ID in a handler, and I cannot think of one. Even if there is one, you can just create N handlers for N events.

Regarding :coeffect - my bad, I just forgot about it.

There's a problem with implicitly adding the event ID to the args map. You won't be able to pass the map to something that expects a strict set of keys. You'll have to be very careful and to dissoc that key manually each time, even though you haven't added it yourself. A particularly unpleasant example - if you don't realize that the event ID key is there, use your own :event-id key (just a plain keyword, without the namespace), and then pass it to something that uses clj->js, you gonna have a bad time:

cljs.user=> (clj->js {:x 1, :a/x 2})
#js {:x 2}

If someone needs to have the event ID there in the handler, then that need is very specific and explicit. I don't think it should be generalized to all handlers. There can be an interceptor that just injects the event ID into the coeffects. It's simple, it's explicit, there's 0 chance of unknowingly messing something up.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 3, 2020

@p-himik
In the example I provided earlier, it turns out to be very useful to have the event id in the event. It allows for the two-step process explained (which generalises to more sophisticated examples involving async HTTP). Ie. there are cases where you want to be able to re-dispatch without knowing what event handler you are in, but to have the event cycle back.

BTW, the intention is that key will be namespaced: :re-frame.core/event-id and not :event-id

@p-himik
Copy link
Contributor

p-himik commented Sep 3, 2020

@mike-thompson-day8 OK, now I've seen such an example. :) And that's a very specific one indeed.
Do you not think that adding an interceptor in that case would be justifiable, especially if that means that all other event handlers that don't need the event ID will not get some implicit information that might potentially mess up with them?

@thheller
Copy link
Contributor

thheller commented Sep 3, 2020

I do agree that having the :re-frame.core/event-id in the map itself is very useful at times, even if just for debugging purposes. A quick dissoc (or helper fn) for cases where you want clj->js or so shouldn't be too hard for the users.

Another option however maybe not putting the event-id into the map and instead use metadata

;; in dispatch, instead of assoc
(vary-meta args assoc ::event-id event-id)

;; ending up with
^{:re-frame.core/event-id :my.app/foo!} {:with "args"}

I personally don't like using meta too much but it is an option.

@p-himik
Copy link
Contributor

p-himik commented Sep 3, 2020

Using dissoc is not too hard indeed. My whole point is not how hard something is but rather about stating that explicit is better than implicit with all other things being equal.

What are the problems in removing the event ID completely from the event arguments and moving it to an optional interceptor? Right now, I don't see any.

@superstructor
Copy link
Contributor

superstructor commented Sep 3, 2020

tldr; I'm pretty hesitant to break with the precedent set by vectors that the event-id is co-located with event 'args' in the same data structure.

This is only in response to taking event-id out of the map entirely, not an argument against the convenience of (dispatch :event-id {:args...}) would would just assoc the event-id into the args map.


The precedent set by event vectors is co-location of the event-id and 'args' in the same data-structure throughout the entire data flow of the app.

While adding event maps is a new thing, we're also keeping full backwards compatibility with vectors. There is a case for keeping these two mechanisms as similar as possible.

In terms of event maps, co-location could be either assoc into the event map like:

{re-frame.core/event-id :event-id :foo 1}`

Or it could be metadata as suggested by @thheller

There are also implementation details that means co-location of the event-id and the event map (aka 'args') makes things a bit simpler. E.g. 10x tracing, re-frame.events/handle etc. Not impossible without sure, but I'd argue more complex.

I hear @p-himik that we could move to being more explicit about the event-id being provided to a handler by an interceptor. Putting aside the precedent set by vectors for a moment lets just think about the implementation of that interceptor.

We'd presumably change re-frame.events/handle to take a nested data structure like {:re-frame.core/event-id :event-id :args {:foo 1 }} and promote :args to the event that is actually dispatched through handlers with (interceptor/execute event interceptors) (event by now being just :args, without event-id).

Now how does the new inject-event-id interceptor get the event-id ? Oh, we'll need to pass the event-id down through interceptors... suddenly we're right back where we started co-locating event-id or doing a lot of complex passing around event-id as extra state just to avoid it being in the 'args' map.

@p-himik
Copy link
Contributor

p-himik commented Sep 3, 2020

I'm pretty hesitant to break with the precedent set by vectors that the event-id is co-located with event 'args' in the same data structure.

I don't understand this reason in the context of us breaking the precedent of using vectors for everything here.

Not impossible without sure, but I'd argue more complex [to implement].

I'm arguing strictly from the perspective of users. As long as they don't notice it at all, it shouldn't be an issue IMO.

Now how does the new inject-event-id interceptor get the event-id ? Oh, we'll need to pass the event-id down through interceptors... suddenly we're right back where we started co-locating event-id or doing a lot of complex passing around event-id as extra state just to avoid it being in the 'args' map.

I don't see any problem or the need for some complex passing here assuming we pass around a map like {:id :my-event, :args {:foo 1}}. Just as before, an interceptor would get a context and return a context. Only now that context would have a nested map instead of a flat vector. An updated version of db-handler->interceptor and its friends then extract the :args map and call the handler function with it. BTW a nice consequence here is that :args doesn't really have to be a map anymore - it can be anything a user wants. A map, a vector, a set, whatever.
All what the new inject-event-id interceptor would do is to (get-in context [:coeffects :event :id]) and to put it somewhere where the handler can see it - probably right in the :args to allow all event handler varieties to get the event ID.

@vvvvalvalval
Copy link

The main downside I see is that it will become harder to consume events for users.

It might be worth it (I'm not debating that here), but if we do go down the road of making events polymorphic, please do make a generic accessor function (re-frame.core/event-id my-event) @mike-thompson-day8.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 4, 2020

@thheller
I can't go with this "view sugar", even though it delivers a pleasingly simple result:

(dispatch :some-event)
(dispatch :some-event {:with "args"})

That's because there is no equally pleasing way to replicate the sugar in effects:

{:db        (assoc db :show-twirly? true)
 :dispatch  ???}       <--  what goes here

??? (above) probably has to be a map literal (no sugar) {:re-frame.core/event-id :some-event :with "args"} which means there's sugar one way, but not the other. Multiple ways of doing something just multiples cognitive load.

same when :fx is used:

{:db  (...)
 :fx  [ [:dispatch ???]
         ...more effects here...]

So dispatching in views (with a sugary white lie) will be different to dispatching in effects (true map litteral ). That kind of inconsistency is never good.

So, I've now swung back to thinkinig I just do map letterals everywhere (no special sugar in views).

(dispatch {::rf/event-id :some-event :with "args"})

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 4, 2020

@p-himik
I'm not 100% sure of your concern, so I'll repeat back to you what I think you are saying ... please correct any misunderstanding.

Imagine a scenario where a button is pressed to add a customer. The event dispatched is a vector:

(dispatch [:add-customer {...}])

where {...} is a map which contains the customer to add - perhaps it might be {:id 1234 :name "Big Corp" :address "blah rd"}

You like the current ability to spec check that {...}, which represents the customer. You like that there is no :event-id key in there.

Now this example was using a vector for the event. But if a map was used instead, you want a way to separate the event from the bag of other attributes, in those cases?

If I have understood (sorry if I haven't), then what would be wrong with this event:

(dispatch {:rf/event-id :add-customer  :customer {...}})

Such an event would package up a map (shown again as {...}) representing the customer which you could then spec check.

BTW, I strongly suspect that your objection might now disappear because we are now NOT going to use the view sugar (dispatch :delete-customer {...}). I think your concern was that this sugary version might encourage people to mistakenly mix the event-id key/value into the {...} part which would then fail spec checks using strict keys.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 4, 2020

@vvvvalvalval

... harder to consume events for users.

By this, do you mean harder to read for programmers? Harder to understand? Or some other form of consumption (or user)?

@p-himik
Copy link
Contributor

p-himik commented Sep 4, 2020

@mike-thompson-day8

You like the current ability to spec check that {...}

No, I didn't say anything about spec'ing or checking. spec is rather orthogonal to this discussion, the way I see it.

You like that there is no :event-id key in there.

Yes, but it should be generalized - re-frame should not change the arguments that I want to pass to event handlers, in any way. Some would argue that changing metadata would be acceptable - I don't have any opinion here since I haven't used metadata that much. But I know that it may be error-prone because not every function preserves metadata.

what would be wrong with this event

Nothing is wrong in terms of the data manipulation.
But if you cared about re-frame not changing your arguments, would you really prefer this form to the "lukewarm coffee" mentioned in the OP? I would certainly not, because it adds verbosity for the sake of verbosity in this particular case - it doesn't solve any problem.

As a side note - in your response to thheller you say: "Multiple ways of doing something just multiples cognitive load".
But this particular issue is all about introducing a new way of doing something that's doesn't solve anything that the old way couldn't solve.

The more I think about, the more that "lukewarm coffee" solution grows on me. There's 0 new cognitive load, 0 chance to break any existing library that relies on events being vectors, and a built-in great flexibility. That "white index lie" where the event ID goes first and all the arguments go after it is not sufficiently bad to be able to justify any changes by itself. From my perspective, it's almost the same as a map entry - it's also an indexed collection, always of two elements, where the key always goes first and the value goes second. It's not a {:key :a, :value 1} kind of thing.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 4, 2020

@p-himik
Yeah, I agree, I too have been thinking more on the "lukewarm coffee" approach.

(dispatch [:event-id {:with "args"}])

You still dispatch a vector. But it is a 2-vector consisting of an event-id element, followed by a map of "the extras".

This sure has one massive advantage - it requires no technical change at all. Indeed, it is already best practice for some re-frame programmers. (I'm afraid I haven't been as smart as they are). The only change would be that I'd have to modify the docs to describe it as a useful "convention" for reasons X and Y and Z.

It does have the problem of "more nesting". That map is inside the vector. And that leads to a more complicated destructuring within the event handler. And it is a little more ugly when I do my "re-dispatch trick" (in the 2-step "weak example" I gave above).

@p-himik
Copy link
Contributor

p-himik commented Sep 4, 2020

@mike-thompson-day8
To be honest, I'm still not sold on the "re-dispatch trick". In your particular example, you already know the event ID because it's static. If it's dynamic - well, just use whatever was passed as the first argument to the reg-event-* function. I think it's impossible to have a situation where not having an event ID inside the event arguments vector somehow makes something unfeasible or even a bit hard to implement.

Regarding more destructuring and making updating the event arguments uglier - there can be an interceptor for that. I already use trim-v for virtually all my event handlers. There can be a trim-vm or something like that that also unfolds the vector and passes just its second element to the handler function. Or maybe even unfolds the vector in full, discards the first element, and calls apply on it and the handler function to allow for multiple arities. This way, updating the arguments is trivial and redispatching the event is also trivial - you will have to construct a new vector with an already known event ID, just like a view would do.

@vvvvalvalval
Copy link

@vvvvalvalval

... harder to consume events for users.

By this, do you mean harder to read for programmers? Harder to understand? Or some other form of consumption (or user)?

@mike-thompson-day8 sorry, to clarify: harder for programmers using re-frame to write generic event-consuming code.

For example: I've written effects that are parameterized with 'callback event prefixes': at the end of its execution, the effect will dispatch an event by appending a value at the end of the 'event prefix' vector. This would no longer work in general, as the callback event may now be a map, and the way to fill it would be assoc-ing into it.

Not saying this is re-frame's fault, but that's one situation where generic processing of events becomes more complex due to re-frame events becoming polymorphic in their data structure.

@thheller
Copy link
Contributor

thheller commented Sep 4, 2020

@mike-thompson-day8 funny enough thats the same issue I'm currently mulling over in my framework. I actually want events themselves to be "declarative" when defining the DOM. So instead of

[:div.foo {:on-click #(dispatch [::some-event! {:with "args"}])} "hello world"]

I just have

[:div.foo {:on-click [::some-event! {:with "args"}]} "hello world"]
;; slightly uglier with maps
[:div.foo {:on-click {::sg/event-id ::some-event! :with "args"}} "hello world"]

Since I'm not constrained by react I like this much more than having actual functions, especially due to the affordances I get during DOM reconciliation. There is little chance I'm going back to functions after doing this for a while.

I've even considered the approach of an uneven length vector being turned into a map (1 for event-id, +N K/V pairs). I don't think it provides much over the "lukewarm coffee" variant but might be something to consider. Harder to provide backwards compatibility though since not all vectors with uneven length will be usable as a map.

[:div.foo {:on-click [::some-event! :with "args"]} "hello world"]

The last approach is just using a regular function call but that sort of negates the aspect of trying to be declarative. Kinda looks worst overall in the hiccup context.

[:div.foo {:on-click (sg/event ::some-event! {:with "args"}} "hello world"]

I think however it would be fine in re-frame since you are already in a "code" context and not in hiccup land. So analog to dispatch and subscribe you'd just have a event function meant to only construct events to handled elsewhere.

{:db        (assoc db :show-twirly? true)
 :dispatch  (rf/event ::some-event! {:with "args"}}   

I don't know what to do for re-frame. Luckily I don't have to worry about staying backwards compatible with anything. Still not and easy choice though.

I do think that maps are strictly better in every regard on the event handling side. I do remember seeing a lot of (fn [env [_ id]] ...) style destructuring in re-frame apps which always looked off to me. Its not terrible and you do get used to it but remembering the argument order is tedious and brittle as you know since thats what motivated all this discussion.

@mike-thompson-day8
Copy link
Contributor Author

mike-thompson-day8 commented Sep 4, 2020

@thheller
Yeah, I totally agree that "callback-as-data" is a good idea, particularly in the context of crossing service worker boundaries which, of course, your framework is all about. (Now, you may be doing this already ...) If I was starting again, .... I'd also be specifically targeting "shared workers" and allowing apps to have multiple GUIs, like neo.mjs. As I understand it, Shared workers were going to be a big deal a few years ago but then Safari refused to adopt them, but I get the feeling they might be coming back and when they do ... everyone will want to be using them.

Unfortunately, React is an increasingly tangled mess which makes Components focal - what with Suspense and Hooks - and it won't be able to be pulled apart and put into separate workers. Your framework approach, with the various parts teased out into workers, looks the much better approach! I'll follow your progress with interest. :-)

@thheller
Copy link
Contributor

thheller commented Sep 5, 2020

Yeah, I played with SharedWorkers but given their state I opted not to use them for the UI. IMHO the neo.mjs takes this way too far too. First of all you now have a single thread responsible for multiple tabs, which means it is even more performance constrained than before. Also DOM related things should stay in the main thread, moving VDOM to a worker is not practical and prohibits a large chunk of important stuff ever being performant enough. As seen in the horrid performance of some of their examples. Unfortunately moving stuff into a worker has its own issues and Browsers do very little to optimize them.

Did you ever experiment with how re-frame would look when things were async? Thats the main issue that needs to be solved in order for Workers to work for anything. What happens if there is no sync @(rf/subscribe [::gimme-data 1])? Suspense is actually nice as a concept but yeah Hooks is where I lost interest in React.

Back on topic I think I'm about to settle on the simplest concept I can think of.

[:div.foo {:on-click {:e ::some-event! :with "args"}} "hello world"]
;; respectively
[:div.foo {:on-click #(dispatch {:e ::some-event! :with "args"})} "hello world"]

I think this was only bugging me (and you) because of trying to use a namespaced descriptive keyword and it getting too long and repetitive. In this context it doesn't actually matter though. :e in event maps being a reserved keyword is actually fine and an uncommon keyword to use anywhere else anyways. dispatch could also just take the :e and replace it with the proper keyword before passing along the event. Not sure about the :e name, might be :x or :i or even go into special places ala :! or :$ to be more generic towards dispatch and subscribe type use cases. Any really short keyword makes this look acceptable IMHO and there is a lot of precedent with :> and so on.

@mainej
Copy link

mainej commented Sep 5, 2020

@thheller this is getting off topic, but out of curiosity, how will you handle e.preventDefault, e.target.value and the like?

@thheller
Copy link
Contributor

thheller commented Sep 5, 2020

@mainej events are always dispatched to the component first. If that component does not handle the event it keeps going up the component tree to the root. The event handler signature is (fn [env event dom-event]) where event is currently a vector but will become the map. dom-event is optional for custom events dispatched "up" but is provided for all events originating from the DOM. Components either handle the event directly or dispatch to the worker (where dom-event is dropped because its not serializable). The component macro has some sugar for events, looks something like this but is going to change a bit when switching to maps.

@danielytics
Copy link

danielytics commented Sep 6, 2020

I personally like the simplicity of the existing way, as, I tend to use wrapped JS components where I convert vectors into calls to dispatch and I like the simplicity of terseness of the vectors: [ui/some-widget {:on-some-event [:event {...}]}] (like @thheller's example), and I use this pattern a lot. Its also nicely terse in :fx.

With that said, on the handler side, I don't really like that I basically always have [_ ...]. What if the handler is always passed only the data .... I understand this breaks the symmetry between dispatch and handler, but is this really a problem in real life? As a user, I don't care about the event-id in a typical handler, because I've already specified it in (reg-event-* <event-id> ...) (and if I want some generic function, I can wrap it, eg: (reg-event-* <event-id> (partial generic-fn <event-id>))

If using a vector is limiting internally, there is nothing wrong with dispatch modifying it however necessary, so long as, as @p-himik said, this is hidden from the user, in the most common cases.

Alternatively, if you really do want to go with maps, but are retaining backwards compatibility anyway (as has been said), then the :fx concern doesn't really matter: just use the old way for :fx. (In which case you may want to transform internally into a canonical form -- but then the handler symmetry is broken. Again, I'm in favour of handlers always getting only the data, so breaking the symmetry isn't a concern for me).

If I do need to use maps and type ::rf/event-id, why not just call it ::rf/id (no e or anything else, and have queries similarly just use ::rf/id)? Its clear (to me at least) that its the event ID from the fact that its an event passed to dispatch (or a query because you are passing it to subscribe). Its certainly no less clear than the existing way where the first item in the vector is the event ID. I'm also not against @thheller's suggestion of using a reserved non-namespaced keyword, as long as its something unlikely to occur in most event data.

Those are the thoughts I had while reading this thread, at least. 🤷

superstructor added a commit that referenced this issue Sep 7, 2020
@mrrodriguez
Copy link

#644 (comment)

I agree with this. I do like the idea of events being maps. However it concerns me that in large codebase it could become error prone to have vectors and maps mix and match since the handlers now have to be specific to one of the other.

I’ve also had a good number of event handlers that dispatch to other event handlers. These will be other points that are easy to overlook.

@beders
Copy link

beders commented Sep 10, 2020

Is it important to distinguish between ::rf/eid and ::rf/qid ?

Currently you can't visually tell an event vector from a subscription vector (other than by adopting naming conventions for subs and events, which is what I'm currently doing. All keywords used for events are in ::events and all subs are in ::subs).

Using eid and qid makes it clear what a map is supposed to be used for. However, is that important?
If not: ::rf/id could be a simpler to grok candidate.

Other variants: :re-frame.subs/id and :re-frame.events/id ?

(BTW, I also like the idea of reducing keyword usage and go with direct function refs):

(dispatch #'do-something arg1 arg2 arg3)

Makes it much easier to navigate the code and prevents mis-typing keywords. I've yet to see why keywords are better candidates for indirection compared to symbols. (happy to be enlightened!)

@olivergeorge
Copy link
Contributor

olivergeorge commented Sep 28, 2020

Throwing my very late 2c in.

A configurable dispatch-fn takes the pressure off picking names here. The event could become any type of data and *dispatch-fn* is the mechanism to pick the event handler which should process it.

(def default-dispatch-fn [x]
  (cond (map? x) (::event-id x)
        (vector? x) (first x)))

(def ^:dynamic *dispatch-fn* default-dispatch-fn)

Possible complication is libs which need to construct their own dispatch calls - how would they know what key to use? If that's an important use case then perhaps the map key needs to be exposed. The 2-arity dispatch might help too.

(def ^:dynamic *dispatch-key* ::event-id)

(defn dispatch-fn [x]
  (cond (map? x) (get x *dispatch-key*)
        (vector? (first x)))

(defn dispatch
  ([e] (queue e))
  ([id e] (dispatch (assoc e *dispatch-key* id)))

@olivergeorge
Copy link
Contributor

Opening up to supporting anything might open up new audiences. Specifically, JS object support might reduce friction in regular JS components/libs.

The dispatch function would be important for this to work.

@clyfe
Copy link

clyfe commented Dec 19, 2020

I fear this feature just complicates things and brings little value-add.

The main rationale here is connasance of name being weaker than connasance of position.
https://connascence.io/pages/about.html

That aside, let's note that Re-Frame events look like fn calls (if you squint):

[::my/event arg1 arg2] ;; <=> (my/event arg1 arg2)

Let's reimagine fn call as maps:

{:fn :defn
 :name ::make-todo
 :args {:title title, :due due}
 :body {:fn :hash-map :1 :title :2 title :3 :due :4 due}}

{:fn ::make-todo, :title "Buy milk", :due "today"} ;; => '{:title "Buy milk", :due "today"}

{:fn :+, :1 3, :2 5} ;; => 8

If there's no value-add in the bit above, why would there be in event maps?
One can always make a map first arg: [::my/event m]; done.

(rf/reg-event-fx
 :do-calc
 (fn [{:keys [db]}
       [event-id {:keys [flushed?] :or {flushed? false}} :as event]]
   (if-not flushed?
      ;; step 1 - 
     {:db        (assoc db :twirly true)
      :dispatch  ^:flush-dom (assoc-in event [1 :flushed?] true)}  ;; <-- adding to the map

      ;; step 2 - hog the CPU
     {:db (do-some-cpu-intensive-task db)})))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests