-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Repl API #10
base: lamdera-next
Are you sure you want to change the base?
Repl API #10
Conversation
I used some code from |
ff12080
to
15a7069
Compare
6dd95ad
to
07ead2f
Compare
…al) to call Endpoint.Package.handlePost: -- Client Elm app says that the package was accepted, but I find no change.
…ndPackageList: - Do this on receipt of the message ExecuteDelayedFunction - This is activated by delayCmd = Process.sleep delayInMs |> Task.perform (always Types.ExecuteDelayedFunction) in function Notebook.Package.updateElmJsonDependencies
…re added (in extra/Endpoint/Package.hs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jxxcarlson – looking good overall. If you could tackle this feedback when you can, I'll do a rebase of the branch after you're done.
@@ -130,6 +146,9 @@ runWithRoot root (Flags maybePort) = | |||
<|> route [ ("_r/:endpoint", Live.serveRpc liveState port) ] | |||
<|> Live.openEditorHandler root | |||
<|> Live.serveExperimental root | |||
<|> (SnapCore.path "repl" $ Repl.endpoint artifactRef) | |||
<|> (SnapCore.path "packageList" $ Package.handlePost artifactRef) | |||
<|> (SnapCore.path "reportOnInstalledPackages" $ Package.reportOnInstalledPackages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we know what's needed here, I think it might be nice to namespace this as follows:
repl
think it makes sense to rename this torepl/js
- the repl endpoint specifically for generating JS, because maybe we'll have other modes in futurepackageList
->repl/setPackages
assuming this better reflects what this endpoint does? But now thinking of it, it probably makes sense for this functionality to be merged intorepl/js
– so you can specify the packages and what you want to compile in one hit, avoiding a race condition if different people want to set different packages and compile different files simultaneously?reportOnInstalledPackages
thus probably also should get merged, or maybe it's not longer required if we can be sure the packages we specify will be the context our Elm will get compiled in?
instance FromJSON Dependencies | ||
|
||
--- curl -X POST -H "Content-Length: 0" http://localhost:8000/reportOnInstalledPackages | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing the endpoint that returns a list of installed packages, for example:
curl -X POST -H "Content-Length: 0" http://localhost:8000/reportOnInstalledPackages
[{"name":"elm/parser","version":"1.1.0"},{"name":"elm/core","version":"1.0.5"}]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I not add comments explaining the purpose of this file?
…ackage.sh and extrra/EndPoin
outlines/repl/elm.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be there? It is changed when a notebook user adds a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jxxcarlson yeah this is part of what we need to figure out how to virtualise entirely.
So that the expected dependencies are always sent with every request, and our server side doesn't need to write it to disk first. Inevitably the elm.json
is read into a value somewhere, and then compilation continues with that values. So if we can figure that out and skip all the disk write/read stuff, that's much better all round and will also prevent race conditions when multiple people try submit things at the same time.
worker/src/Endpoint/Compile.hs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this reference to port 8007?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep for CORS issues locally we need it.
- use to wire in Backend.Session.reconnect and Backend.Session.sendUserData
857ae96
to
433ec5e
Compare
3f4f772
to
7b19439
Compare
To boot:
Then to test with curl:
After any changes run
:rr
to recompile + reboot the lamdera live server.The Javascript output uses postmessage to communicate with the current window: https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage - not in particular the listener registration – this is how your JS code would receive the message from the evaluated JS.
As to how to evaluate the JS within the API result: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval
If you want to do this all in Elm from elm-notebook, I would suggest:
/repl
command from Elm, constructing the equivalent request as above, expecting the body as aString
Ok string
, send that out to Javascript with a porteval(string)
.value
is the ANSI string of the execution result as above.I think this will be the core building block POC.
Errors
Here's an example request that returns an Elm compiler error object instead:
This seems pretty straightforward and I guess it'll mainly be a matter of the right decoder and then a display output. Hopefully we can reverse engineer that from the compiler error JSON type being constructed! Alternatively - I have a strong suspicion that @dillonkearns has already done this in elm-pages... maybe he'll have time to comment :)