funcool / promesa

A promise library & concurrency toolkit for Clojure and ClojureScript.
https://funcool.github.io/promesa/latest/
Mozilla Public License 2.0
498 stars 58 forks source link

p/doseq #112

Closed borkdude closed 2 years ago

borkdude commented 2 years ago

I often find users trying to use doseq with promises which isn't a good combination since doseq is synchronous. Perhaps it's possible to come up with a promesa-variant of this.

niwinz commented 2 years ago

what is the use case? I mean I use promesa extensively and never needed it. An example would be awesome.

borkdude commented 2 years ago

E.g.:

(let [links ["https://foobar.net" "https://foobar.com"]]
  (doseq [l link]
    (playwright/screenshot l #js {:path "/tmp/screenshot.png"})))

Using p/doseq these things would actually happen in serial.

Perhaps there's already another way to do this. p/loop is one of them.

I'm also pinging @joe-loco here who was running into this today.

niwinz commented 2 years ago

Yeah, I understand.

I'm happy to accept a PR for the promises aware doseq.

borkdude commented 2 years ago

One problem with doseq is that the expansion is pretty involved because you can have multiple bindings and :when and :let but 99% of the time people use only one binding + seq and the involved expansion may not what we want for performance reasons either.

This is an implementation in user space:

(ns doseq
  {:clj-kondo/config '{:lint-as {doseq/pdoseq clojure.core/doseq}}}
  (:require [promesa.core :as p]))

(defmacro pdoseq
  "Simplified version of `doseq` which takes one binding and a seq, and
  runs over it using `p/run!`"
  [[binding xs] & body]
  `(p/run! (fn [~binding]
               (p/do ~@body))
           ~xs))

(pdoseq [x [1 2 3]]
        (prn x)
        (p/delay 1000))

I'd personally be fine with this limited version, but it might raise the expectation that it supports of all doseq.

niwinz commented 2 years ago

I'm pretty fine with the limited version. I think I have never used the all the other stuff of doseq and I tend to replace it with run! all the times on my code for performance reasons.

niwinz commented 2 years ago

So with your permission, I will take this doseq impl for promesa.

borkdude commented 2 years ago

Yeah. As long as the docstring documents the limitation, I think it's fine.

borkdude commented 2 years ago

I'll provide a PR

borkdude commented 2 years ago

@niwinz See PR. I noticed (or rather clj-kondo noticed) that there was an unused value here:

https://github.com/borkdude/promesa/commit/3b9b92fff513b7b08a9349a32848062d43dbdca3#diff-3252c28c90776426b8ed0bd8100b3c709cba322ef141909ac3fec13e6ba2839bR493

niwinz commented 2 years ago

Feel free to remove the value, In any case, personally I don't care on linter issues on test code. Thanks for the PR, looks perfect. I will merge it and try to solve the other issue before version release with some other additional changes to the promesa.exec namespace. I also have options to experiment with jdk19 virtual threands and maybe introduce some stuff on promesa hehe

borkdude commented 2 years ago

Perhaps the unused value was intended to wrapped in a test assertion?