lambda-toolshed / papillon

Interceptor library for Clojure
Eclipse Public License 1.0
53 stars 3 forks source link

nbb support #2

Open jsg24 opened 1 year ago

jsg24 commented 1 year ago

Hi there,

How close/what would be involved to offer nbb support?

Given that there won't be core.async support , would it be possible to only include the promises, dependencies and code on the cljs path?

Thanks in advance!

stevenproctor commented 1 year ago

Right now, we piggyback ReadPort protocol, so not sure what it might take to give a new protocol.

I have been working with @cch1 on some other updates, so I will continue to think about how we might support this.

dmg46664 commented 1 year ago

@stevenproctor Hi, we went ahead an took the Papillon code and rewrote it for a promise style interface. I.e. No required surrounding go blocks, and so much simpler for this use-case!

Perhaps we can get around to extract it from our code base and posting it as a library. The grind is real 😅

stevenproctor commented 1 year ago

@dmg46664 @jsg24 I have pushed an update on #6 for nbb, which now uses a new protocol instead of ReadPort. Note: there may be a renaming of the protocol and method coming, but the concept still stands.

This would allow for Native JS Promises (covered out of the box for CLJS and nbb) and support extending Promesa, Manifold, or other libraries if you use other forms of async behavior in nbb.

I also updated that branch to have a way to take the generic async result and present it as a specific type of result for async returns; in nbb, I have it return a native JS Promise when the returned result is async instead of always returning a channel.

This again could be overridden to give a function that would take the Chrysalis protocol and turn it into a Promesa promise, Manifold result type, or something else.

I just pinged for feedback from the other core maintainers to get their thoughts on any refinements/cleanup/gotchas from their end, but I would like to know if this route is an ergonomic solution for your use cases.

Not that you need to dispose of what you did, but maybe if we cover it well enough, it can help the next set of people looking towards using nbb as their Clojure runtime.

dmg46664 commented 1 year ago

@stevenproctor Great stuff! I don't mind disposing of our stuff at all if we can swap yours in (it came from yours anyway, which was a pleasure to alter) 😆

Just to be clear, when your branch is merged your execute will then return a promise and require no go block? In that case I see no reason not to change back to your library! 🏄

The only other thing we improved from our departure point is a heuristic check that the interceptors are in fact returning a context (and equivalent on the leave) ⚠️ However, looking at your code now I see it has moved on some.

(defn enter [ctx]
  (cond
    (p/promise? ctx)
    (p/then ctx #(enter %))

    (:lambda-toolshed.papillon/error ctx)
    ctx

    (or (-> ctx map? not) (-> ctx :lambda-toolshed.papillon/queue nil?))
    (throw (ex-info "An Interceptor :enter fn in the queue has returned an invalid context." {}))

    :else (let [queue (:lambda-toolshed.papillon/queue ctx)]
            (if (empty? queue)
              ctx
              (let [ix (peek queue)
                    new-queue (pop queue)
                    new-stack (conj (:lambda-toolshed.papillon/stack ctx) ix)]
                (recur (-> ctx
                           (assoc :lambda-toolshed.papillon/queue new-queue
                                  :lambda-toolshed.papillon/stack new-stack)
                           (try-stage ix :enter))))))))
dmg46664 commented 1 year ago

Digesting more of your comment and the branch sited, you've provided some default behaviour as well as a way to present the async result differently by a mapping in the context. I don't think there is any difference between a promise and a promesa promise once received (despite differences in creation) so I imagine this will just work...

I wasn't aware of nbb reader conditionals, nice one!

cch1 commented 1 year ago

@dmg46664 , much of the "moving on" was work to do exactly what you noted: validate that interceptors are returning contexts (from enter, leave and error) instead of something inadvertent. See https://github.com/lambda-toolshed/papillon/commit/915c49da3dd7fbf7c6f4a8f3e7fac2c5124d000e for the essentials.

only other thing we improved from our departure point is a heuristic check that the interceptors are in fact returning a context (and equivalent on the leave) ⚠️ However, looking at your code now I see it has moved on some.

dmg46664 commented 1 year ago

@cch1 Great! We'll await the branch getting merged then 👏