Clojurians Log v2

@djblue PR opened. Had a couple questions too in the comments https://github.com/djblue/portal/pull/19

@nate to your second question, maybe we can have a lower level api that is only available for server backed rpc implementations. Like first you (portal.launcher/start host port) , then you (portal.api/open) as many times as you like? I think it's simpler but definitely not as easy 😬

ah, that makes a lot of sense

so if you call (portal.api/open), you get the default behavior and if you want to specify the host/port, then you need to run (portal.launcher/start host port) first?

That's my thinking, would that be reasonable?

I think so

and would this only be necessary for the jvm and node launchers?

I guess it could also be portal.api/start

With a description about how it's a low level thing that you only need to care about if you want control over the server

yeah, portal.api/start sounds good

I'll update the PR with this later

Sounds great, thanks!

you're welcome, thank you for the feedback

@djblue I was working on this yesterday morning and realized that only port is unchangeable (and should be passed to the start function). host is something that can be different on each open call. Is it confusing to have :portal.colors/theme and :portal.launcher/host as options for open and only honor :portal.launcher/port in start?

the tricky part about making start the way to override host is that it's not an attribute of the server, so I would need to store it in the atom as well so that open can use it

I'm leaning toward host and port being just for start, because that seems less confusing, thoughts?

@nate I agree, keeping those options for start now seems best

cool, thank you

Apologies on the delay in getting this done, work rather exploded this week. I should have some time on Saturday to get the PR updated.

@djblue I just pushed a commit that separates out the a new start function. I'm sure of the JVM launcher code. I'm less sure of the Node launcher code as I was only able to test it by running the node sample (I couldn't connect my editor to the repl and start/stop, but I was able to stop via the actual REPL prompt). Please let me know what you think of the code and if there's anything I should change. Thank you.

@nate, to test different environments, I use make e2e . It looks like you might have missed fixing p/close for node. You can run make e2e/node specifically to run those tests.

The main issues I'm seeing is that the node process isn't terminating which probably means the server is still running.

diff --git a/src/portal/runtime/node/launcher.cljs b/src/portal/runtime/node/launcher.cljs
index 05f89fe..80926b9 100644
--- a/src/portal/runtime/node/launcher.cljs
+++ b/src/portal/runtime/node/launcher.cljs
@@ -47,14 +47,12 @@
 
 (defn start [options]
   (when (nil? @server)
-    (reset!
-     server
-     (a/let [{:portal.launcher/keys [port host] :or {port 0 host "localhost"}} options
-             server (create-server #'server/handler port host)]
-       server))))
+    (a/let [{:portal.launcher/keys [port host] :or {port 0 host "localhost"}} options
+            instance (create-server #'server/handler port host)]
+      (reset! server instance))))
 
 (defn- stop [handle]
-  (some-> handle :http-server (.close)))
+  (some-> handle :http-server .close))
 
 (defn open [options]
   (let [session-id (random-uuid)]

@nate ☝️ should fix it. a/let returns a promise

Ah! Ok. Thanks for that. I'll add that and run the e2e tests.

@djblue I just pushed up the above code change as a commit. Looks like the jvm, bb, and node e2e tests run well. I can't get e2e/web to work.

You might have to enable pop-ups but it's fine, everything looks good to me locally.

@nate I just merged the PR, thanks! I should have a release out in the next couple of days.

awesome, thank you for merging

and thank you for your feedback during the process