Can try to reuse a closed browser

[vemv]

Hey there!

Thank you for your project.

I followed https://github.com/pfeodrippe/wally/tree/f6a6979731cdd37ad7ba36fa88cd4a4f411fd686#usage , and for the sake of experimentation, I closed the browser and tried to perform the same steps again.

The first step (w/navigate "https://clojars.org/metosin/jsonista") failed with:

Execution error (DriverException) at com.microsoft.playwright.impl.Connection/dispatch (Connection.java:201).
Error {
  message='Target page, context or browser has been closed
  name='Error
  stack='Error: Target page, context or browser has been closed
    at DispatcherConnection.dispatch (/private/var/folders/yc/h50n8fgn0h79y848sfkr942h0000gn/T/playwright-java-17403553361898517108/package/lib/server/dispatchers/dispatcher.js:232:49)
    at transport.onmessage (/private/var/folders/yc/h50n8fgn0h79y848sfkr942h0000gn/T/playwright-java-17403553361898517108/package/lib/cli/driver.js:50:57)
    at Immediate._onImmediate (/private/var/folders/yc/h50n8fgn0h79y848sfkr942h0000gn/T/playwright-java-17403553361898517108/package/lib/protocol/transport.js:77:34)
    at process.processImmediate (node:internal/timers:476:21)

Perhaps it's worthwhile / feasible to...

• inspect browser state in advance prior to interacting with it? and/or
• clear Wally's internal state when encountering this, and possibly other issues?

Thanks - V

[vemv]

@pfeodrippe: to be honest I don't mind much about this particular issue, it's just that it made the README snippet less robust, which isn't a good look!

However, I'm finding the following issue more practical:

• One can forget to wrap code with wally/with-page, in which case, the default chromium-based page will be used
  • this can easily be very undesirable, e.g. create hard-to-debug issues, or false negatives in tests (the test is passing because it's running in Chromium, not Firefox, hiding cross-compat bugs)

I would suggest either of two solutions:

• remove the default Chromium-based page
  • will make the README snippet slightly less nice, but more serious usage more robust, IMO
• Remove the dynamic var, make a page argument mandatory throughout all of the API
  • this is more modern Clojure, since dyn vars predate core.async (which isn't quite compatible with dyn vars), and also have other problems (performance, and more)
  • but the API could become quite more verbose without an alternative, recommended pattern.

Such an alternative pattern could be e.g.:

(-> (wally/make-page)
    (doto (w/fill "//*[@id=\"loginForm\"]/div/div[1]/div/label/input"
                  "my-username"))
    (doto (w/fill "//*[@id=\"loginForm\"]/div/div[2]/div/label/input"
                  "some-password")))

Which removes the need for repeating a page argument.

I wouldn't enjoy being too pushy - just leaving these for your consideration!

Cheers - V

[pfeodrippe]

@pfeodrippe: to be honest I don't mind much about this particular issue, it's just that it made the README snippet less robust, which isn't a good look!

However, I'm finding the following issue more practical:

• One can forget to wrap code with wally/with-page, in which case, the default chromium-based page will be used

  • this can easily be very undesirable, e.g. create hard-to-debug issues, or false negatives in tests (the test is passing because it's running in Chromium, not Firefox, hiding cross-compat bugs)

I would suggest either of two solutions:

• remove the default Chromium-based page

  • will make the README snippet slightly less nice, but more serious usage more robust, IMO

• Remove the dynamic var, make a page argument mandatory throughout all of the API

  • this is more modern Clojure, since dyn vars predate core.async (which isn't quite compatible with dyn vars), and also have other problems (performance, and more)
  • but the API could become quite more verbose without an alternative, recommended pattern.

Such an alternative pattern could be e.g.:

(-> (wally/make-page)
    (doto (w/fill "//*[@id=\"loginForm\"]/div/div[1]/div/label/input"
                  "my-username"))
    (doto (w/fill "//*[@id=\"loginForm\"]/div/div[2]/div/label/input"
                  "some-password")))

Which removes the need for repeating a page argument.

I wouldn't enjoy being too pushy - just leaving these for your consideration!

Cheers - V

Thanks for the suggestions!!

I guess making a page argument mandatory would make it less easy for beginners, and you can always use with-page as you mentioned o/ We can add some flag (opt-in) that complains if you don't use with-page, wdyt? Would like to do a PR for adding this flag?

I guess any performance issue for *page* being dynamic is very negligible as we are dealing with browsers, and the browser interaction is what will. Why dynamic vars predating core.async is a problem?

[vemv]

Hey! Sorry for the delay.

We can add some flag (opt-in) that complains if you don't use with-page, wdyt?

Yes, for instance would seem satisfactory to me:

(defonce ^:dynamic *page*
  (if (#{"true" "1"} (System/getProperty "pfeodrippe.wally.no-default-page"))
    nil
    (make-page)))

I guess any performance issue for page being dynamic is very negligible as we are dealing with browsers, and the browser interaction is what will.

Yes, that's entirely true. I just pointed it out as one of the usual cons which, as I have perceived it, have made dynvars less "modern" as of recent years.

Why dynamic vars predating core.async is a problem?

Say one integrated core.async with Wally, it might become less usable, since dynamic bindings are lost across go blocks. I'm not tremendously concerned about this use case, but it's another "usual con".

Anyway, I thought of another, more practical issue: dyn vars propagate across vanilla java threads. While playwright-java is not thread-safe. So a dyn var makes unsafe code more likely to happen, e.g.:

(wally/with-page page
  (pmap (fn [x]
          (wally/foo x)) ;; this is thread-unsafe, and the underying dynamic binding makes it easy to forget that
        xs))

I created #5 to expand on that.

Cheers - V

[jave]

I also scratched my head a bit over this, so some documentation or something would help.

[jave]

(w/with-page (w/make-page)
;; When some command is run for the first time, Playwright
;; will kick in and open a browser.
(w/navigate "https://clojars.org/metosin/jsonista")
(w/click [(ws/text "Copy") (ws/nth= "1")])

(w/navigate "https://clojars.org/metosin/jsonista")
(w/click [(ws/text "Copy") (ws/nth= "1")])

(w/fill :#search "reitit")
(w/keyboard-press "Enter")
(w/click (sel/a (sel/attr= :href "/metosin/reitit")))
(.textContent (w/-query (ws/text "Downloads")))

)

If you do the demo like this instead, you can reliable re-run the demo, by closing the brower window after each run. However, if you just re-run without closing, you get an exception. so its still not very good for a demo

[pfeodrippe]

Yes, need to get back on this, guess I will review @vemv’s PR this weekend, thanks for the feedback

[jave]

FWIW my preference would be to add the page argument explicitly, for me it makes it easier to think about the code.