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

nbb support #2

Open
jsg24 opened this issue Jan 30, 2023 · 8 comments
Open

nbb support #2

jsg24 opened this issue Jan 30, 2023 · 8 comments

Comments

@jsg24
Copy link

jsg24 commented Jan 30, 2023

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

@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
Copy link

@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
Copy link

dmg46664 commented Feb 28, 2023

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
Copy link
Contributor

cch1 commented Feb 28, 2023

@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 915c49d 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
Copy link

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

@cch1
Copy link
Contributor

cch1 commented Oct 16, 2024

Branch is merged!

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

4 participants