clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Reconsider `:aleph/`-namespaced keywords #700

Open DerGuteMoritz opened 7 months ago

DerGuteMoritz commented 7 months ago

Problem

Keywords with an :aleph/ namespace are currently used inconsistently: Sometimes the namespace means that the keys are internal (e.g. :aleph/channel), sometimes they are intended for public use and only namespaced to prevent clashes with other keys in the same map (e.g. :aleph/request-arrived).

See https://github.com/clj-commons/aleph/pull/699#discussion_r1403507539 and https://github.com/clj-commons/aleph/pull/699#discussion_r1403545356 for prior discussion which lead to the creation of this issue.

Solution

Still to be determined. One idea we had was to use a different namespace for keys which are indeed not intended for public use (e.g. :aleph.internal/).

Also, we should document :aleph/ keys which are indeed intended for public use.

KingMob commented 7 months ago

One thing we could do is stick all internal keys in a map under a single key, like:

{:headers {...}
 :body ...
 :aleph/keep-alive? true
 :aleph/request-arrived some-nano-time
 :aleph/internal {:ch ch 
                  :gory-internal1 ...
                  :gory-internal2 ...}}

I think it's nice and tidy this way.

DerGuteMoritz commented 7 months ago

I think I slightly prefer the flat version because looking up a single internal key is more convenient then (most usage sites are only interested in one of them at a time I think). But the submap approach has the nice advantage that endusers can just dissoc that single key to rid the request map of the gory internals, so there's that.

Another alternative could be to store these in metadata (cf. how it's done for :aleph/channel on streams in the UDP, TCP and websocket implementations).

KingMob commented 7 months ago

Flat version is fine, I don't care that much.

Please do not store them in metadata though. I've been wanting to remove that behavior because it violates expectations about what metadata is, and what it's attached to.