fable-compiler / repl-legacy

http://fable.io/repl
MIT License
14 stars 10 forks source link

[WIP][Top Secret] Working on new version of repl #8

Closed MangelMaxime closed 6 years ago

MangelMaxime commented 6 years ago

This PR is top secret nothing to see here :)

Ok, will stop here my mysterious intro and show you a gif:

repl_demo_v2

As you can guess, now the repl is executing the code inside an iframe. Which mean we now have access to the DOM. This is really a WIP and I need to do lot of clean up and also I want to add more features. I mostly wanted to use this PR to share my progress and avoid being two to work on the next REPL version.

I am basing this PR on the #7 to have a better diff. Will try to keep the branch sync and apply the different patch where they belongs. Fixing architecture of the repo goes to #7 where updating the REPL UX goes here.

Only big thing missing to merge #7 is the autocomplete support as it's a regression. The UglifyJS thing can be solve later as it's not blocking.

PS: Yes, I consider using the React-monaco components but it don't feel ready yet and don't allow us enough flexibility. So no need to add it for now :)

alfonsogarciacaro commented 6 years ago

AMAZING!!! Keep up the awesomeness!

ezgif com-optimize

MangelMaxime commented 6 years ago

We now have three editors:

repl_layout_2

Next steps are:

I really need to find another GIF recorder under linux. This one output really large file and also looks likes it's playing speed x2-3...

MangelMaxime commented 6 years ago

Hello guys,

would it be possible for you to test the current state of the REPL in this PR ? You can see the features added in my previous message.

Idea is to get feedback on the resize/collapse, auto-complete, build/run features.

I will later remove the initial state with No apps compiled yet or make it have a better display is a little rough in current state.

PS: The REPL is build with the latest master branch of the different repos.

alfonsogarciacaro commented 6 years ago

This is amazing @MangelMaxime!!! :clap: :clap: :clap:

I gave it a try but I got an error. I don't have SSH installed, maybe https can be used instead? https://github.com/ncave/FSharp.Compiler.Service.git

Running build with 1 worker
Starting Target: Setup
Can't find FSharp.Compiler.Service_fable at: /Users/alfonsogarciacaronunez/dev/fable-repl/..
Do you want me to setup it for you ? (O/N)
O
Installing FSharp.Compiler.Service_fable for you
git clone git@github.com:ncave/FSharp.Compiler.Service.git FSharp.Compiler.Service_fable
ο»ΏRunning build failed.
Error:
System.Exception: Cloning into 'FSharp.Compiler.Service_fable'...
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I love that I have to write O for yes ;)

MangelMaxime commented 6 years ago

Grr.. I hate https :D

One thing that surprise me is why is it testing your SSH key when the repo are publics.

But if the script can support it why not yes. And yes, it's possible that O means Oui :D Also you can also do ./build.sh All setup=auto to avoid the questions :)

alfonsogarciacaro commented 6 years ago

I cloned ncave's FCS manually, checked out fable branch and seems to work but I got a StackOverflow exception at the end of the JS compilation. This happened to me before, I'll try on Windows :)

MangelMaxime commented 6 years ago

Strange I don't have any SO since I started this PR. Perhaps something new has been added to the latest version of the fable branch.

StachuDotNet commented 6 years ago

Running into the same issue when executing './build.cmd All setup=auto':

C:\Program Files\Git\cmd\git.exe clone git@github.com:ncave/FSharp.Compiler.Service.git FSharp.Compiler.Servi
ce_fable
Running build failed.
Error:
System.Exception: Cloning into 'FSharp.Compiler.Service_fable'...
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
   at Fake.Git.CommandHelper.gitCommand(String repositoryDir, String command) in D:\code\FAKE\src\app\FakeLib
\Git\CommandHelper.fs:line 67
   at Fake.Git.Repository.clone(String workingDir, String repoUrl, String toPath) in D:\code\FAKE\src\app\Fak
eLib\Git\Repository.fs:line 15
   at FSI_0005.Build.ensureRepoSetup(RepoSetupInfo info)
   at FSI_0005.Build.clo@95.Invoke(Unit _arg1)
   at Fake.TargetHelper.runSingleTarget(TargetTemplate`1 target) in D:\code\FAKE\src\app\FakeLib\TargetHelper
.fs:line 626

---------------------------------------------------------------------
Build Time Report
---------------------------------------------------------------------
Target     Duration
------     --------
Setup      Failure
---------------------------------------------------------------------
Status:    Failure
alfonsogarciacaro commented 6 years ago

After some tries I finally managed to complete the build (on Windows and using Fable development version) but I may be missing something because I get errors when running the REPL πŸ€”

image

MangelMaxime commented 6 years ago

@alfonsogarciacaro In general, to run the repl I do: ./build.sh Build.FCS and then yarn run start-app

This should be enough, when you have the folders already setup. I just upgraded the paket version to use Fable 1.3.4 because a replacement was missing for me.

And indeed, seems like something change on the FCS or Fable side. Probably the FCS side as it's the one related to type checking, etc.

Also, for me Fable is generated the right code and the sample is running fine: screenshot from 2017-11-29 13-07-14

I edited, the code to force a re-evaluation of the file. Perhaps, @ncave will have more infos about the bugs we have now.

Edit:

To complete this part:

And indeed, seems like something change on the FCS or Fable side. Probably the FCS side as it's the one related to type checking, etc.

What's strange is the FCS branch has not changed since my last check and only the Fable repo and Repl repo have new commits in it...

MangelMaxime commented 6 years ago

Is the current layout good enough ? Or should I already add support to something like shown in @mike-morr comments ?

alfonsogarciacaro commented 6 years ago

I think the layout is great! When we get it working the next step probably is to offer a way to save and share snippets, and later add a file explorer if we support multiple files.

alfonsogarciacaro commented 6 years ago

@MangelMaxime I think we should merge this, keep working on master and publish a beta version of the REPL for this repository so people can try it and give feedback. If you still want to be able to keep pushing comments you can be in charge of master and the rest of us use PRs :+1:

BTW, as @ncave suggested I confirmed that the performance when using net45 assemblies is better so maybe we should be using those for now. I'm also planning to move the checker to a worker so it doesn't freeze the UI. I'll send a PR :)

MangelMaxime commented 6 years ago

@alfonsogarciacaro Yes, we can merge this PR into master directly I think and close #7 too.

And, I will use issues + PR to keep tracks of what we are working on. So it should be easier than having a list of task into a single PR :).

I just don't know how to generate the net45 assemblies would you be able to update this PR with them ? And after that we merge this PR.

ncave commented 6 years ago

@MangelMaxime

I just don't know how to generate the net45 assemblies

You already have them here, these should be good enough for now.

MangelMaxime commented 6 years ago

Ah yes that's true :) thanks you @ncave

MangelMaxime commented 6 years ago

For info, I am working on the last WIP feature before merging this PR.

And for me, speaking from a pure user experience point of view, I don't see a lot of differences between the old/new binaries. I didn't make any time performance measure yet.

ncave commented 6 years ago

@MangelMaxime If you use a bigger file, you'll see it right away, they scale differently with size.

MangelMaxime commented 6 years ago

@alfonsogarciacaro I can't make ExportDefault works...

I commented it in this commit: https://github.com/fable-compiler/fable-repl/pull/8/commits/951996c1634816660f4da1e8c117bd30af76d529#diff-b5ce1565c2555faa5e0618ae5c1fcd3d

When, I try to do:

let editor : Fable.Editor.Interfaces.IExports = importDefault "editor"
console.log editor

I got undefined, can you please help me :) ?

alfonsogarciacaro commented 6 years ago

@MangelMaxime Sorry, I don't understand. In the commit, ExportDefault is commented out.

alfonsogarciacaro commented 6 years ago

@MangelMaxime You use ExportDefault as in here (well, in that sample you could also put the attribute directly on top of defaultManager) then you import it as in here. Note the path must follow the rules for JS importing. In this case, if you want to use an alias instead of a relative path, you have to declare the alias in the webpack config (note in this example the fable-repl is exposed as a global variable as it is loaded directly by index.html).

MangelMaxime commented 6 years ago

@alfonsogarciacaro I will create a branch with the code uncommented. Because, i think I did what you said.

ncave commented 6 years ago

@MangelMaxime @alfonsogarciacaro Just in time for the holidays, some performance improvements for the netcore binaries.

Happy Holidays and best wishes to you and your families!

MangelMaxime commented 6 years ago

Awesome @ncave , just in time for me to make the next version of the repl public :) Doing some clean up :)

Happy holidays and best wishes to you too. πŸŽ… 🎁

alfonsogarciacaro commented 6 years ago

Fantastic @ncave :clap: πŸ₯‡ :clap:

Thanks a lot for all your work with Fable. I wish you a great and well-deserved holidays! Let's get new energies to make 2018 a great year for Fable! πŸ’ͺ

MangelMaxime commented 6 years ago

Here is my Christmas gift:

πŸŽ‰ 🍾 🎁 http://fable.io/fable-repl/ 🎁 🍾 πŸŽ‰

Thank to both of you for the help :) Still a lot of thing to do, but I will stop here for now :) I will create issue to track the next task I have in mind and so we will be able to keep track of it and work together when needed.

@alfonsogarciacaro If this is working for you too, I will let you make an public announcement with FableCompiler twitter if we want to make it public :) (Someone already liked the WIP PR ^^)

ncave commented 6 years ago

@MangelMaxime Awesome, thanks for merging into master. If any of the stale branches are no longer needed, they can be cleaned up.

alfonsogarciacaro commented 6 years ago

Absolutely awesome @MangelMaxime! ⭐️ ⭐️ You're a Fable superstar!! ⭐️ ⭐️

Applause

It's a bit late now so we better make the announcement tomorrow. Totally agreed, let's keep separated issues for minor improvements :+1:

alfonsogarciacaro commented 6 years ago

And this is for you @ncave!

MangelMaxime commented 6 years ago

@alfonsogarciacaro Yes, totally agree doing an announcement at 1 AM don't work well in general :)