SDK: refactor ChannelOwner.post!/3 -> Channel.post/3

The `ChannelOwner.post!/3` implementation was problematic in (at least) the following ways: - It resulted in a somewhat inheritance-like implementation and usage, w/out any need for that; - It did not handle well differences between "posts" that should result in updates to the "subject" resource vs. retrieval of related resources; - It did note handle well cases in which the "subject" **should not** be refreshed. These shortcomings required multiple work-arounds and code verbosity. The introduction of `Channel.post/3` feels much cleaner, more manageable, and idiomatic.
mechanical-orchard · Sep 30, 2024 · 26fed18 · 26fed18
1 parent 19b9dd1
commit 26fed18
Show file tree

Hide file tree

Showing 23 changed files with 558 additions and 447 deletions.
diff --git a/lib/playwright.ex b/lib/playwright.ex
@@ -18,6 +18,8 @@ defmodule Playwright do
   """
 
   use Playwright.SDK.ChannelOwner
+  alias Playwright
+  alias Playwright.SDK.Channel
   alias Playwright.SDK.Config
 
   @property :chromium
@@ -85,16 +87,21 @@ defmodule Playwright do
 
   defp new_browser(session, client, options)
        when is_atom(client) and client in [:chromium, :firefox, :webkit] do
-    with play <- Playwright.SDK.Channel.find(session, {:guid, "Playwright"}),
+    with play <- Channel.find(session, {:guid, "Playwright"}),
          guid <- Map.get(play, client)[:guid] do
-      {:ok, Playwright.SDK.Channel.post(session, {:guid, guid}, :launch, options)}
+      playwright = %Playwright{
+        guid: guid,
+        session: session
+      }
+
+      {:ok, Channel.post({playwright, :launch}, options)}
     end
   end
 
   defp new_session(transport, args) do
     DynamicSupervisor.start_child(
-      Playwright.SDK.Channel.Session.Supervisor,
-      {Playwright.SDK.Channel.Session, {transport, args}}
+      Channel.Session.Supervisor,
+      {Channel.Session, {transport, args}}
     )
   end
 end
diff --git a/lib/playwright/api_request_context.ex b/lib/playwright/api_request_context.ex
@@ -9,8 +9,12 @@ defmodule Playwright.APIRequestContext do
   """
 
   use Playwright.SDK.ChannelOwner
+  alias Playwright.SDK.Channel
   alias Playwright.APIRequestContext
 
+  # types
+  # ----------------------------------------------------------------------------
+
   @type fetch_options() :: %{
           optional(:params) => any(),
           optional(:method) => binary(),
@@ -24,6 +28,9 @@ defmodule Playwright.APIRequestContext do
           optional(:ignore_HTTPS_errors) => boolean()
         }
 
+  # API
+  # ----------------------------------------------------------------------------
+
   # @spec delete(t(), binary(), options()) :: APIResponse.t()
   # def delete(context, url, options \\ %{})
 
@@ -43,19 +50,8 @@ defmodule Playwright.APIRequestContext do
   # def patch(context, url, options \\ %{})
 
   @spec post(t(), binary(), fetch_options()) :: Playwright.APIResponse.t()
-  def post(%APIRequestContext{session: session} = context, url, options \\ %{}) do
-    Channel.post(
-      session,
-      {:guid, context.guid},
-      :fetch,
-      Map.merge(
-        %{
-          url: url,
-          method: "POST"
-        },
-        options
-      )
-    )
+  def post(%APIRequestContext{} = context, url, options \\ %{}) do
+    Channel.post({context, :fetch}, %{url: url, method: "POST"}, options)
   end
 
   # @spec put(t(), binary(), options()) :: APIResponse.t()
@@ -66,9 +62,7 @@ defmodule Playwright.APIRequestContext do
 
   # TODO: move to `APIResponse.body`, probably.
   @spec body(t(), Playwright.APIResponse.t()) :: any()
-  def body(%APIRequestContext{session: session} = context, response) do
-    Channel.post(session, {:guid, context.guid}, :fetch_response_body, %{
-      fetchUid: response.fetchUid
-    })
+  def body(%APIRequestContext{} = context, response) do
+    Channel.post({context, :fetch_response_body}, %{fetchUid: response.fetchUid})
   end
 end
diff --git a/lib/playwright/binding_call.ex b/lib/playwright/binding_call.ex
@@ -2,7 +2,8 @@ defmodule Playwright.BindingCall do
   @moduledoc false
   use Playwright.SDK.ChannelOwner
   alias Playwright.BindingCall
-  alias Playwright.SDK.{Channel, Helpers}
+  alias Playwright.SDK.Channel
+  alias Playwright.SDK.Helpers.Serialization
 
   @property :args
   @property :frame
@@ -19,8 +20,9 @@ defmodule Playwright.BindingCall do
         page: "TBD"
       }
 
-      result = func.(source, Helpers.Serialization.deserialize(binding_call.args))
-      Channel.post(session, {:guid, binding_call.guid}, :resolve, %{result: Helpers.Serialization.serialize(result)})
+      Channel.post({binding_call, :resolve}, %{
+        result: Serialization.serialize(func.(source, Serialization.deserialize(binding_call.args)))
+      })
     end)
   end
 end
diff --git a/lib/playwright/browser.ex b/lib/playwright/browser.ex
@@ -67,8 +67,7 @@ defmodule Playwright.Browser do
   def close(%Browser{session: session} = browser) do
     case Channel.find(session, {:guid, browser.guid}, %{timeout: 10}) do
       %Browser{} ->
-        Channel.post(session, {:guid, browser.guid}, :close)
-        :ok
+        Channel.close(browser)
 
       {:error, _} ->
         :ok
@@ -124,14 +123,14 @@ defmodule Playwright.Browser do
 
   ## Arguments
 
-  | key/name         | type   |             | description |
+  | key/name           | type   |             | description |
   | ------------------ | ------ | ----------- | ----------- |
   | `accept_downloads` | option | `boolean()` | Whether to automatically download all the attachments. If false, all the downloads are canceled. `(default: false)` |
   | `...`              | option | `...`       | ... |
   """
   @spec new_context(t(), options()) :: BrowserContext.t()
-  def new_context(%Browser{guid: guid} = browser, options \\ %{}) do
-    Channel.post(browser.session, {:guid, guid}, :new_context, prepare(options))
+  def new_context(%Browser{} = browser, options \\ %{}) do
+    Channel.post({browser, :new_context}, prepare(options))
   end
 
   @doc """

diff --git a/lib/playwright/browser_context.ex b/lib/playwright/browser_context.ex
@@ -253,7 +253,7 @@ defmodule Playwright.BrowserContext do
   def add_cookies(context, cookies)
 
   def add_cookies(%BrowserContext{} = context, cookies) do
-    post!(context, :add_cookies, %{cookies: cookies})
+    Channel.post({context, :add_cookies}, %{cookies: cookies})
   end
 
   @doc """
@@ -301,7 +301,7 @@ defmodule Playwright.BrowserContext do
   """
   @spec add_init_script(t(), binary() | map()) :: t()
   def add_init_script(%BrowserContext{} = context, script) when is_binary(script) do
-    post!(context, :add_init_script, %{source: script})
+    Channel.post({context, :add_init_script}, %{source: script})
   end
 
   def add_init_script(%BrowserContext{} = context, %{path: path} = script) when is_map(script) do
@@ -323,12 +323,12 @@ defmodule Playwright.BrowserContext do
   """
   @spec clear_cookies(t()) :: t()
   def clear_cookies(%BrowserContext{} = context) do
-    post!(context, :clear_cookies)
+    Channel.post({context, :clear_cookies})
   end
 
   @spec clear_permissions(t()) :: t()
   def clear_permissions(%BrowserContext{} = context) do
-    post!(context, :clear_permissions)
+    Channel.post({context, :clear_permissions})
   end
 
   @doc """
@@ -339,13 +339,12 @@ defmodule Playwright.BrowserContext do
   > - The default browser context cannot be closed.
   """
   @spec close(t()) :: :ok
-  def close(%BrowserContext{session: session} = context) do
+  def close(%BrowserContext{} = context) do
     # A call to `close` will remove the item from the catalog. `Catalog.find`
     # here ensures that we do not `post` a 2nd `close`.
-    case Channel.find(session, {:guid, context.guid}, %{timeout: 10}) do
+    case Channel.find(context.session, {:guid, context.guid}, %{timeout: 10}) do
       %BrowserContext{} ->
-        Channel.post(session, {:guid, context.guid}, :close)
-        :ok
+        Channel.close(context)
 
       {:error, _} ->
         :ok
@@ -364,13 +363,13 @@ defmodule Playwright.BrowserContext do
 
   ## Arguments
 
-  | key/name | type  |                            | description |
+  | key/name   | type  |                            | description |
   | ---------- | ----- | -------------------------- | ----------- |
   | `urls`     | param | `binary()` or `[binary()]` | List of URLs. `(optional)` |
   """
   @spec cookies(t(), url | [url]) :: [cookie]
-  def cookies(%BrowserContext{session: session} = context, urls \\ []) do
-    Channel.post(session, {:guid, context.guid}, :cookies, %{urls: urls})
+  def cookies(%BrowserContext{} = context, urls \\ []) do
+    Channel.post({context, :cookies}, %{urls: urls})
   end
 
   @doc """
@@ -449,7 +448,7 @@ defmodule Playwright.BrowserContext do
   @spec expose_binding(BrowserContext.t(), String.t(), function(), options()) :: BrowserContext.t()
   def expose_binding(%BrowserContext{session: session} = context, name, callback, options \\ %{}) do
     Channel.patch(session, {:guid, context.guid}, %{bindings: Map.merge(context.bindings, %{name => callback})})
-    post!(context, :expose_binding, Map.merge(%{name: name, needs_handle: false}, options))
+    Channel.post({context, :expose_binding}, Map.merge(%{name: name, needs_handle: false}, options))
   end
 
   @doc """
@@ -471,18 +470,18 @@ defmodule Playwright.BrowserContext do
   @spec grant_permissions(t(), [String.t()], options()) :: t() | {:error, Playwright.API.Error.t()}
   def grant_permissions(%BrowserContext{} = context, permissions, options \\ %{}) do
     params = Map.merge(%{permissions: permissions}, options)
-    post!(context, :grant_permissions, params)
+    Channel.post({context, :grant_permissions}, params)
   end
 
   @spec new_cdp_session(t(), Frame.t() | Page.t()) :: Playwright.CDPSession.t()
   def new_cdp_session(context, owner)
 
-  def new_cdp_session(%BrowserContext{session: session} = context, %Frame{} = frame) do
-    Channel.post(session, {:guid, context.guid}, "newCDPSession", %{frame: %{guid: frame.guid}})
+  def new_cdp_session(%BrowserContext{} = context, %Frame{} = frame) do
+    Channel.post({context, "newCDPSession"}, %{frame: %{guid: frame.guid}})
   end
 
-  def new_cdp_session(%BrowserContext{session: session} = context, %Page{} = page) do
-    Channel.post(session, {:guid, context.guid}, "newCDPSession", %{page: %{guid: page.guid}})
+  def new_cdp_session(%BrowserContext{} = context, %Page{} = page) do
+    Channel.post({context, "newCDPSession"}, %{page: %{guid: page.guid}})
   end
 
   @doc """
@@ -495,10 +494,10 @@ defmodule Playwright.BrowserContext do
   @spec new_page(t()) :: Page.t()
   def new_page(context)
 
-  def new_page(%BrowserContext{session: session} = context) do
+  def new_page(%BrowserContext{} = context) do
     case context.owner_page do
       nil ->
-        Channel.post(session, {:guid, context.guid}, :new_page)
+        Channel.post({context, :new_page})
 
       %Playwright.Page{} ->
         raise(RuntimeError, message: "Please use Playwright.Browser.new_context/1")
@@ -537,7 +536,7 @@ defmodule Playwright.BrowserContext do
       patterns = Helpers.RouteHandler.prepare(routes)
 
       Channel.patch(session, {:guid, context.guid}, %{routes: routes})
-      post!(context, :set_network_interception_patterns, %{patterns: patterns})
+      Channel.post({context, :set_network_interception_patterns}, %{patterns: patterns})
     end)
   end
 
@@ -575,7 +574,7 @@ defmodule Playwright.BrowserContext do
 
   @spec set_offline(t(), boolean()) :: t()
   def set_offline(%BrowserContext{} = context, offline) do
-    post!(context, :set_offline, %{offline: offline})
+    Channel.post({context, :set_offline}, %{offline: offline})
   end
 
   # ---
@@ -585,7 +584,7 @@ defmodule Playwright.BrowserContext do
 
   # ---
 
-  @spec unroute(t(), binary(), function() | nil) :: :ok
+  @spec unroute(t(), binary(), function() | nil) :: t()
   def unroute(%BrowserContext{session: session} = context, pattern, callback \\ nil) do
     with_latest(context, fn context ->
       remaining =
@@ -594,7 +593,6 @@ defmodule Playwright.BrowserContext do
         end)
 
       Channel.patch(session, {:guid, context.guid}, %{routes: remaining})
-      :ok
     end)
   end
 

diff --git a/lib/playwright/browser_type.ex b/lib/playwright/browser_type.ex
@@ -163,7 +163,7 @@ defmodule Playwright.BrowserType do
   # ----------------------------------------------------------------------------
 
   defp browser(%BrowserType{} = browser_type) do
-    Channel.post(browser_type.session, {:guid, browser_type.guid}, :launch, Config.launch_options())
+    Channel.post({browser_type, :launch}, Config.launch_options())
   end
 
   defp chromium(session) do

diff --git a/lib/playwright/cdp_session.ex b/lib/playwright/cdp_session.ex
@@ -27,15 +27,9 @@ defmodule Playwright.CDPSession do
   # API
   # ---------------------------------------------------------------------------
 
-  @spec detach(t()) :: t() | {:error, term()}
-  def detach(%CDPSession{session: session} = cdp_session) do
-    case Channel.post(session, {:guid, cdp_session.guid}, :detach) do
-      {:ok, _} ->
-        cdp_session
-
-      {:error, %Playwright.API.Error{} = error} ->
-        {:error, error}
-    end
+  @spec detach(t()) :: t() | {:error, Playwright.API.Error.t()}
+  def detach(%CDPSession{} = cdp_session) do
+    Channel.post({cdp_session, :detach}, %{refresh: false})
   end
 
   @doc """
@@ -49,8 +43,8 @@ defmodule Playwright.CDPSession do
   end
 
   @spec send(t(), binary(), options()) :: map()
-  def send(%CDPSession{session: session} = cdp_session, method, params \\ %{}) do
-    Channel.post(session, {:guid, cdp_session.guid}, :send, %{method: method, params: params})
+  def send(%CDPSession{} = cdp_session, method, params \\ %{}) do
+    Channel.post({cdp_session, :send}, %{method: method, params: params})
   end
 
   # private