[WIP] Add Suave hosting for FSAC #74

Krzysztof-Cieslak · 2015-10-09T21:39:35Z

Hey,

So this is my huge PR we've discussed through email. Basically it does 3 things -

Split core functionality of FSAC to its own project and separate it from "presentation / communication" layer which allows to create multiple way of providing FSAC functionality.
Update console application to use this core library.
Create Suave server providing same functionalities as console app.

It's still work in progress - no integration tests for Suave app, hard-coded port for Suave server, one "failing" integration test for console app (however I'm not sure how important it is), some random changes in solution files for integration tests (not sure how I've managed to change it) - but it's pretty much fully usable - console application is working exactly the same as current one, and Suave app is providing exactly same functionalities - I've already have Ionide-fsharp fork (https://github.com/ionide/ionide-fsharp/tree/suave) using it and it's working well.

rneatherway · 2015-10-11T19:09:25Z

FsAutoComplete.Suave/FsAutoComplete.Suave.fs

+
+[<EntryPoint>]
+let main argv =
+    let mutable state = FsAutoComplete.State.Initial


Please could you use a ref here instead, so that it compiles with F# 3?

rneatherway · 2015-10-11T19:15:31Z

Wow, this a significant chunk of work, great! I'm very happy to accept such a pull request, with or without tests, hopefully you can find time to add them conveniently at a later date. I have a few comments.

The solution files in the integration tests aren't needed, you can just delete them if you like.
JSonSerializer.fs seems to be duplicated, should in be in Core?
Parser.fs should be deleted from FsAutoComplete
Most of CommandReponse.fs is duplicated (with a slightly different API), I think your version in Core should be kept and the one in FsAutoComplete can just use that.
The unit tests don't pass anymore, perhaps they just don't reference the correct project now.

Krzysztof-Cieslak · 2015-10-13T14:40:01Z

OK, so next part of work:

I've removed CommandReponse.fs and Parser.fs from FsAutoComplete - I've just missed them when I was moving files around, version from Core was used anyway.
I've removed .sln files in integration tests
Have changed mutable state to ref
JsonSerializer.fs is duplicated "on purpose" - I'm planning to investigate different serialization for Suave backend, and also don't think that serialization implementation should be part of Core anyway.
I still have problem with unit tests - I've added app.config to test project and it's now working in VS (still failing when I run it from FAKE). But looking on fail messages, I think I've messed something with bindings redirects / FSharp.Core version reference

Fixes for F# 3

Krzysztof-Cieslak · 2015-10-13T22:03:39Z

Looks like last fix, has broken build on windows 🙄

rneatherway · 2015-10-13T22:25:58Z

Yeah I hate these app.config/versioning issues. FsUnit is sometimes really bad like this, luckily it isn't involved here. New PR sent over.

Use FSharp.Core 4.3.0.0 throughout

rneatherway · 2015-10-14T09:09:58Z

Looking good! How would you like to handle releases? If you add the necessary logic to the FAKE script then I can release both at once to https://github.com/fsharp/FsAutoComplete/releases easily.

Also, this won't impact me merging it, but won't you lose the ability to send unsolicited messages, like errors? Do you have to poll for them now? Is the main benefit for Atom that it has lots of built in machinery for interfacing with HTTP?

Krzysztof-Cieslak · 2015-10-14T13:21:21Z

I actually haven't thought about releases... Probably best way would be to release 2 separate zips for that - one with Console App, second with Suave. I don't know if it might be done as part of one GitHub tag/release or will we need 2?

If error / info is result of command it will be send as (part of) response. And to the other potential errors - haven't seen one since update to 0.24.1 ( and I'm using Atom a lot right now ;) ), before it was just StackOverflow which got fixed (and it crashed app anyway)... so don't think it's a big problem.

Change to HTTP makes clearer request -> response cycle, which makes stuff in Atom ... and other node.js based tools ... easier. I also hope web based server will enable other cool stuff - I know @dsyme mentioned some time ago building new, not Silverlight-based TryFsharp. I guess it can be also usable for @tpetricek projects - FsSnippet (if new version would include "richer" F# editor) or TheGamma which already have web based F# editor.

rneatherway · 2015-10-14T14:07:33Z

I actually haven't thought about releases... Probably best way would be to release 2 separate zips for that - one with Console App, second with Suave. I don't know if it might be done as part of one GitHub tag/release or will we need 2?

For now, yes let's just do two separate zips on the same tag. That makes it really easy to release, and if there are no changes for one or the other of the frontends in a release that's fine, editors don't need to update. Can you see how to adjust the logic in the FAKE script for that?

If error / info is result of command it will be send as (part of) response.

Doesn't this mean the parse command will be blocking? Perhaps Atom is multi-threaded (unlike Emacs!) so this isn't an issue.

rneatherway · 2015-10-14T14:07:46Z

I agree the other stuff sounds great!

Krzysztof-Cieslak · 2015-10-14T14:24:12Z

Can you see how to adjust the logic in the FAKE script for that?

Yep, i will take a look at it. It shouldn't be difficult to add second zip to release.

Doesn't this mean the parse command will be blocking?

HTTP request is always non-blocking in JavaScript, I'm sending a request, get promise of result and just define what happens when it's resolved - it makes things much easier than continue listening on stdio and interpreting incoming from there strings.

rneatherway · 2015-10-14T14:40:46Z

HTTP request is always non-blocking in JavaScript, I'm sending a request, get promise of result and just define what happens when it's resolved - it makes things much easier than continue listening on stdio and interpreting incoming from there strings.

Great, thanks for the explanation. That does sound nice to work with!

Krzysztof-Cieslak · 2015-10-14T15:54:34Z

Haven't tested whole release cycle (as don't have write rights here) but looks like it should be working.

rneatherway · 2015-10-14T19:11:11Z

build.fsx


 let buildDir = project + "/bin/Debug/"
 let buildReleaseDir = project + "/bin/Release/"
 let integrationTestDir = project + "/test/integration/"
 let releaseArchive = "fsautocomplete.zip"

+let suaveSummary = "A Suave web server for interfacing with FSharp.Compiler.Service over a HTTP."
+let suaveProject = "FsAutoComplete.Suave"
+let suaveBuildDebugDir = suaveProject + "/bin/Release"


Should be debug

rneatherway · 2015-10-14T19:28:51Z

Thanks, I'm going to merge and release. I found some strange issue though with it rebuilding FsAutoComplete.Suave each time, because the timestamp for Suave.dll downloaded by Paket was in 2017!

[WIP] Add Suave hosting for FSAC

Krzysztof-Cieslak · 2015-10-14T20:16:20Z

Hurrah! 😂 ;)

rneatherway · 2015-10-14T20:59:59Z

:)

Krzysztof-Cieslak added 5 commits September 26, 2015 18:30

Huge refactoring

d0a99bb

Working suave server

79dc621

Add helptext endpoint

dbba6c0

Fix sln files in integration tests

4e528e2

Add port parameter

6ccf306

rneatherway reviewed Oct 11, 2015
View reviewed changes

Krzysztof-Cieslak added 5 commits October 13, 2015 16:01

Remove unnecessery files

86eb6fc

Add app config for changes

177643b

Update fsproj file

c6b4f24

Change mutable to ref

4f4c13d

Remove sln files from integration tests

ef7a7f6

rneatherway and others added 3 commits October 13, 2015 22:19

Fixes for F# 3

e916f61

Merge pull request #1 from rneatherway/ionide-suave-robin

cc11e0d

Fixes for F# 3

Use FSharp.Core 4.3.0.0 throughout

7e1c898

Merge pull request #2 from rneatherway/ionide-suave-robin

fca228e

Use FSharp.Core 4.3.0.0 throughout

Krzysztof-Cieslak added 2 commits October 14, 2015 17:52

Add assembly info

55eabbb

Update build script

42c5fa6

rneatherway reviewed Oct 14, 2015
View reviewed changes

Fix typo

5aa3cfe

rneatherway added a commit that referenced this pull request Oct 14, 2015

Merge pull request #74 from ionide/suave

fde76f3

[WIP] Add Suave hosting for FSAC

rneatherway merged commit fde76f3 into ionide:master Oct 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add Suave hosting for FSAC #74

[WIP] Add Suave hosting for FSAC #74

Krzysztof-Cieslak commented Oct 9, 2015

rneatherway Oct 11, 2015

Krzysztof-Cieslak Oct 13, 2015

rneatherway commented Oct 11, 2015

Krzysztof-Cieslak commented Oct 13, 2015

Krzysztof-Cieslak commented Oct 13, 2015

rneatherway commented Oct 13, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway Oct 14, 2015

Krzysztof-Cieslak Oct 14, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015

[WIP] Add Suave hosting for FSAC #74

[WIP] Add Suave hosting for FSAC #74

Conversation

Krzysztof-Cieslak commented Oct 9, 2015

rneatherway Oct 11, 2015

Choose a reason for hiding this comment

Krzysztof-Cieslak Oct 13, 2015

Choose a reason for hiding this comment

rneatherway commented Oct 11, 2015

Krzysztof-Cieslak commented Oct 13, 2015

Krzysztof-Cieslak commented Oct 13, 2015

rneatherway commented Oct 13, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway Oct 14, 2015

Choose a reason for hiding this comment

Krzysztof-Cieslak Oct 14, 2015

Choose a reason for hiding this comment

rneatherway commented Oct 14, 2015

Krzysztof-Cieslak commented Oct 14, 2015

rneatherway commented Oct 14, 2015