-
Notifications
You must be signed in to change notification settings - Fork 50
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
[WIP] Add auto-complete suggestions #270
base: master
Are you sure you want to change the base?
[WIP] Add auto-complete suggestions #270
Conversation
3ef84d9
to
a27999c
Compare
a27999c
to
3880d2f
Compare
server/Main.hs
Outdated
get "/complete" $ do | ||
query <- param "q" | ||
Scotty.setHeader "Access-Control-Allow-Origin" "*" | ||
Scotty.setHeader "Content-Type" "application/json" | ||
let ideClient = | ||
Process.createProcess_ "purs-ide-client" | ||
(Process.proc "purs" ["ide", "client"]) | ||
{ Process.std_in = Process.CreatePipe | ||
, Process.std_out = Process.CreatePipe | ||
} | ||
mkCommand q = A.encode $ A.object | ||
[ "command" .= ("complete" :: Text) | ||
, "params" .= A.object | ||
[ "filters" .= A.Array | ||
( V.fromList | ||
[ A.object | ||
[ "filter" .= ("prefix" :: Text) | ||
, "params" .= A.object | ||
[ "search" .= q ] | ||
] | ||
] | ||
) | ||
] | ||
] | ||
(Just handleIn, Just handleOut, _, _) <- liftIO ideClient | ||
liftIO $ Char8.hPutStrLn handleIn (mkCommand (query :: Text)) | ||
result <- liftIO $ BS.hGetContents handleOut | ||
Scotty.text (TL.fromStrict (T.decodeUtf8 result)) |
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.
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.
Maybe this could be a POST request with JSON constructed on the client, and passed directly to purs ide client
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.
It's somehow much faster using POST 7496700, but I don't know if there's any security issues with just proxying directly to purs ide client
/ purs ide server
(probably not since I think purs ide client
will fail on malformed input?)
138ee85
to
7496700
Compare
@nwolverson -- I'm curious if you have any thoughts on this from the language server side of things; I'm not really sure what is and isn't possible with |
As per discussion on discord I'd suggest avoiding proxying the It is surprising that the request would take 2s, it would seem the only extra overhead is an HTTP request, you are spawning the Happy to discuss the details of the completion results themselves later - but that request looks sane at least |
42df7b0
to
0bdd1fd
Compare
This is surprising to me—these are the results from this commit 0bdd1fd (which is back to being a GET request, encoding the JSON on the server)
so, pretty snappy, but removing this line Line 191 in 0bdd1fd
I added that line because the previous iteration with the POST request was hanging and that seemed to fix it, but I don't fully understand the behavior there. Other results for completeness :)From 7496700 POST request (not much difference) Benchmark #1: curl -s 'http://localhost:8081/complete' -X POST --data-binary '{ "command": "complete", "params": { "filters": [{ "filter": "prefix", "params": { "search": "app" } }] } }' -H 'content-type: application/json' Time (mean ± σ): 117.7 ms ± 10.5 ms [User: 4.7 ms, System: 7.0 ms] Range (min … max): 100.6 ms … 137.3 ms 21 runs Running Benchmark #1: echo '{ "command": "complete", "params": { "filters": [{ "filter": "prefix", "params": { "search": "app" } }] } }' | purs ide client -p 8082 Time (mean ± σ): 69.8 ms ± 3.2 ms [User: 7.3 ms, System: 16.2 ms] Range (min … max): 65.1 ms … 81.3 ms 33 runs |
Description of the change
Fixes #32.
Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.
Checklist: