Skip to content
This repository has been archived by the owner on Jun 4, 2022. It is now read-only.

Generalized sock server #105

Merged
merged 34 commits into from
Jul 2, 2017
Merged

Generalized sock server #105

merged 34 commits into from
Jul 2, 2017

Conversation

futuro
Copy link
Contributor

@futuro futuro commented Mar 19, 2017

Here's the initial work to allow the socket server to accept a namespaced function via the CLI and run it when someone connects.

I've got a test namespace in tst/hello/world.cljs that just prints \nHello friend.\n\n and closes the socket.

You can call it with -A 'namespaced.fn/here'

@futuro
Copy link
Contributor Author

futuro commented Apr 10, 2017 •
edited by anmonteiro
Loading

@anmonteiro and I had a conversation on slack where we decided on the following changes to mimic Clojure's socket server behavior. Here's the Clojure Socket Server REPL design doc, and currently the following is going to get done.

  • Remove the -A CLI option
  • Add the ability for the -n (and associated long form) option to accept edn with the following keys: :address, :port, :accept, :args
  • Add error handling where an insufficient number of keys are passed in.
    • What happens if no :port is specified?
    • What if we've got :args but no :accept?
    • :address but no :port?
    • No :address or :port?
  • Add tests
    • Move socket server starting tests from cli-test to cljs-test
    • cljs.js tests
      • Figure out why the lumoEval tests are failing
    • socketRepl.js tests (I don't think there's much that can be tested here, as we don't export anything)
    • cli.js tests
    • repl.cljs tests

Some things I don't believe we can test with jest:

  1. One function calling another function inside a module
  • While jest.spyOn would allow us to maintain the original functionality of the first function and still test whether it gets called, etc., I believe the first function has a closure over the second function's original definition from the import, so any spies/mocks of the second function won't actually get called.
  • It may be possible to adapt jest.mock's ability to mock up a whole module, but I haven't investigated that yet
  1. Any internal variables, so testing for side-effects that change internal vars won't be possible
  • This, too, might be possible to test with jest.mock, but I'm not sure. Preferably we could do something like jest.spyOn('../some-module') and every variable and function from the module would be spied on so we could inspect changes throughout.

So, whatever we want to test, it has to A) be exported and B) can't involve testing non-exported variables/functions. I'm not familiar enough with the JS testing landscape to offer suggestions, but I'll endeavor to test what I can within the constraints of the framework.

@anmonteiro anmonteiro mentioned this pull request May 27, 2017
Open
@futuro futuro force-pushed the generalized-sock-server branch 2 times, most recently from 8c799c6 to 5dbf0fb Compare June 28, 2017 19:37
@anmonteiro anmonteiro force-pushed the generalized-sock-server branch 3 times, most recently from 11797fc to 85c5434 Compare July 1, 2017 00:36
futuro
futuro commented Jul 1, 2017
View reviewed changes
src/cljs/snapshot/lumo/repl.cljs Outdated
@@ -1370,6 +1364,7 @@
(merge opts {:ns (symbol ns-sym)})
(fn [{:keys [ns value error] :as ret}]
(try
(apply value socket fn-args)
;; TODO: do we wanna splice args?
(value socket fn-args)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm thinking about it, we should probably switch value to fn-symbol or something that's representative of what it holds.

futuro added 20 commits July 1, 2017 18:04
@futuro @anmonteiro
Generalize socket server accept function
9ecff4e 
The current implementation of the socket server is built specifically around
running a repl, instead of allowing other possible functions to handle input
from the socket. I've mostly pulled out the repl specific code into it's own
function, and allowed for passing an `accept` function (following the same naming
convention as clojure.core.server) to the `open` function in socketRepl.js.

There's still a call to unhookOutputStreams inside of the `close` function that
I believe isn't necessary since this returns the stdout/stdin for the process to
the original value, but we're calling it as the process is being shut down, so
that's cleaned up anyways.

More importantly, any cleanup for a particular accept function should happen
inside that accept function, as we won't be able to generalize the socket server otherwise.
@futuro @anmonteiro
Cleaned up comments
0850b28 
I wanted to clarify some design decisions so I cleaned up the comments a bit.
@futuro @anmonteiro
Fixed lint errors
0dbe277 
We're not using weak types like Function or any, so I've created an AcceptFn
type for the accept functions.
@futuro @anmonteiro
Changed openRepl's return type to void
b6ebeb5 
Since we can't use weak return types I believe I need to decide on a return type
for every accept function. Since currently we don't use the return value of the
accept function, and I can't currently think of a reason for an accept function
to have a return value, I've set the default return type to void.

I've tested these changes locally against eslint and flow, so hopefully I've
covered all of the bases this time.
@futuro @anmonteiro
Fixed broken tests
6432a58 
I've actually fixed the broken tests and walked through the steps the CI will
do, so this commit should run properly.
@futuro @anmonteiro
Moved Socket REPL startup into cljs init function
56f1aea 
If we want to resolve ClojureScript functions we'll need to have access to the
ClojureScriptContext, which means the Socket REPL initialize has to happen after
the ClojureScript engine is started.

Since the startClojureScriptEngine starts the engine as well as running whatever
scripts, main functions, etc. that the user has started, it made sense to put
the Socket REPL code there as well.
@futuro @anmonteiro
CLJS functions can now receive the socket
23f96b4 
This means we can now define an arbitrary cljs function as the accept function.

I've mimicked the work in run-main for resolving the namespace the function is
in, and then running it.
@futuro @anmonteiro
Added cli option to specify an accept function
5cb4f85 
You can now specify a namespaced CLJS function which will be called when a
client connects. It can currently accept a single argument, which is a socket.
@futuro @anmonteiro
Fixed some flow issues in cljs.js
77cfd8e
@futuro @anmonteiro
Added socket REPL arg handling
8d60abd 
We can now process JSON args, host:port, and port specifications with the -n
flag.
@futuro @anmonteiro
Updated repl error handler in run-accept-fn
95cbe29 
The repl error handler changed when I rebased, so now it's up to date.
@futuro @anmonteiro
Added extra call to initClojureScriptEngine
6767897 
For some reason I need to init the cljs engine twice for the repl to
start. TODO: figure out why.
@futuro @anmonteiro
Initial work handling accept fn args
7cad62e 
The parsing is done, and now I'm working on getting the args to parse
properly.

When I `JSON.parse` the args in JS then pass them to CLJS via the
`ClojureScriptContext.lumo.repl.run_accept_fn.call` in cljs.js, it turns
into some form of object that `js->clj` can't parse into a cljs map (at
least, println doesn't print it properly, but console.log does).

If I run `(js->clj (js/JSON.parse (js/JSON.stringify (first args))))`
in lumo/repl.cljs then everything comes out fine. I'm not sure why this
is failing...
@futuro @anmonteiro
added type notifications to some vars in cljs.js
05686ee
@futuro @anmonteiro
removed extraneous log statements from socketRepl.js
5814c5f
@futuro @anmonteiro
Worked around odd object conversion issue
c025f71 
When building the dev scripts, for some reason passing a JSON object
from the JS code to the CLJS code means we can't convert to cljs objects
with `js->cljs`. This apparently worked if you build the release
executable, but that leaves dev hard to develop/test/debug. So I've
introduced a workaround.
@futuro @anmonteiro
Updated hello/world.cljs now passed args back over the socket.
242b0d4
@futuro @anmonteiro
Fixed replOpts check
046a565 
Previously I was just checking for the instantiation of replOpts, as a
determiner for whether we should pull it apart to get the host/port/etc.
Now this works as expected.
@futuro @anmonteiro
Added tests for srepl opt parsing errors
345adc7 
If you pass bad data to the Socket REPL it should throw a SyntaxError.
@futuro @anmonteiro
finished cljs tests
35a13e8
futuro and others added 14 commits July 1, 2017 18:04
@futuro @anmonteiro
moved socketRepl startup tests to cljs-test
7b2dc1f
@futuro @anmonteiro
updated help text snapshot
448c758
@futuro @anmonteiro
Converted startCLJS to async function
83e8eef 
We're using promises to ensure errors in the socket repl don't blow up
the whole repl, so to ensure the server is started before printing it's
details we're using async/await.

Not that I think about it, this should probably go into the socketREPL
namespace, which would change the far reaching nature of these changes.
Hmmm...
@futuro @anmonteiro
moved socket repl output test to cljs-test
0023751
@futuro @anmonteiro
updated -h snapshot in cli-test
4d56bee
@futuro @anmonteiro
added missing snapshot for cljs-test
de160f6
@futuro @anmonteiro
updated startCLJS tests to be async compatible
07455cd
@anmonteiro
first tweaks, fix tests
38fa19d
@anmonteiro
tweak help message
ce84f2e
@anmonteiro
babel external helpers plugin
00c6a23
@anmonteiro
wip
8b4e351
@anmonteiro
wip
1e8bec2
@anmonteiro
fix windows CI
d079472
@anmonteiro
actually fix windows ci
7709e3a
@anmonteiro anmonteiro merged commit 452ef43 into anmonteiro:master Jul 2, 2017
@anmonteiro anmonteiro deleted the generalized-sock-server branch July 2, 2017 04:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@futuro @anmonteiro