search

fable-compiler / repl-legacy

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

Use artifact from AppVeyor #10

Closed alfonsogarciacaro closed 6 years ago

alfonsogarciacaro commented 6 years ago

Ok, after some head scratching I got this working (more or less). Great job @MangelMaxime! I haven't modified the build script much but to test this you can:

Compilation, error highlighting and autocompletion is working. But there are still a few parts missing:

Please have a look and tell me if you know how can we move forward from here :+1:

screen shot 2017-12-03 at 6 04 06 pm
MangelMaxime commented 6 years ago

Thanks for the PR I will review/test it locally tomorrow.

I believe we should be able to remove the watch-app scripts because you can use directly start-app to use the webpack-dev-server.

Don't touch my beautiful localhost:8080 lines :smiling_imp:

No more seriously, I will make the host auto-discover before making the REPL online (I will add a TODO in the right PR). Basically the generator.fs is used to generate Blob files which are then passed to the iframe. This allow, us to serve the Html + JavaScript for the iframe without having a backend server hosting temp files etc.

alfonsogarciacaro commented 6 years ago

Sorry, @ncave, any idea why the Fable.Import.Browser.dll reference is not working? Unfortunately this prevents using the REPL to interact with canvas or any other HTML element.

ncave commented 6 years ago

@alfonsogarciacaro Looks ok to me, it works here. Note: the column pos seems to be zero-based. Once you publish a version with this fix, it should be fine.

alfonsogarciacaro commented 6 years ago

@ncave Would you mind checking out branch feature/new_repl2 of this repo and running the following?

git clean -xfd      # just in case
sh build.sh          # or build.cmd on Windows
yarn start

When I try to compile the canvas sample code in the REPL, besides the red squiggles in anything referencing Fable.Import.Browser with the message "internal error: Cannot read property 'compilingFslib' of undefined (Failure)" on hovering, this is the result of JS compilation (anything else that doesn't reference Fable.Import.Browser.dll works):

export function init() {
  let canvas;
  throw 1;
  throw 1;
  throw 1;
  let ctx;
  throw 1;
  throw 1;
  throw 1;
  throw 1;
  throw 1;
}
init();
ncave commented 6 years ago

@alfonsogarciacaro Not sure about that, as I'm not all that familiar with the Browser bindings.

Changing this line: let canvas = Browser.document.getElementsByTagName_canvas().[0] with this one: let canvas = Browser.document.createElement_canvas() makes it work (I know it's not the same, it's just an example).

So not all of Fable.Import.Browser.dll is bad, it must be something else.

P.S. There is a minor build issue when building on tabula rasa:

   ERROR in ./src/Monaco/util.js
    Module not found: Error: Can't resolve '../../../Fable/src/js/fable-utils/babel-plugins' 
in 'C:\Projects\fable-repl\src\Monaco'
     @ ./src/Monaco/util.js 5:0-80

probably needs to be replaced with local reference.

alfonsogarciacaro commented 6 years ago

Awesome! It's indeed working if I use your suggestion:

let canvas = Browser.document.createElement_canvas()
Browser.document.body.appendChild(canvas) |> ignore

I can even run the Ozmo game! :tada: So you're right it must be something else, and it seems the problem is methods with Emit attribute. Do you know if somehow attribute info is partially missing in the metadata assemblies?

About the build error, I don't remember seeing it but I updated npm dependencies and added fable-utils explicitly in case it fixes the error.

BTW @MangelMaxime the problem with the ozmo game is the keyboard events are not captured by the iframe. Is there any solution for this? Would be an option to remove the iframe?

BTW2 @ncave the autocompletion in the REPL is very slow when working on a long piece of code. Is there a way to parse the code without generating the AST, as FSAC does?

MangelMaxime commented 6 years ago

I will take a look at why the events are not working probably something to do with how iframes work etc. But it's should work for example, the elm mario debugger sample is using iframe too and they have keyboard support.

Also to test the REPL, I am actually using this piece of code which allow me to not create a canvas directly. Creating a canvas via JS is harder than we think because, the size, ratio etc. need to be set too.

But every any case, we need to fix the Emit attributes from DLL if it's the problem.

I will take a look at this PR and when I have publish Thot beta version. Taking longer that I expected but I should do it today.

ncave commented 6 years ago

@alfonsogarciacaro

Is there a way to parse the code without generating the AST, as FSAC does?

Do you have a link to how FSAC does it, or are you suggesting to not do as FSAC does it?

ncave commented 6 years ago

@alfonsogarciacaro One possible (temporary) solution is to switch to the old metadata binaries until performance on the netcore binaries can be improved (there is a bottleneck somewhere that needs to be profiled, see #1205).

ncave commented 6 years ago

@alfonsogarciacaro

... it seems the problem is methods with Emit attribute...

No, that's when it shows, but is actually caused by #1291.

alfonsogarciacaro commented 6 years ago

@ncave

Do you have a link to how FSAC does it, or are you suggesting to not do as FSAC does it?

AFAIK, to get the AST you need to set keepAssemblyContents to true so I assume FSAC is just not setting it. But I don't see such an option with the InteractiveChecker.

One possible (temporary) solution is to switch to the old metadata binaries until performance on the netcore binaries can be improved.

Yes, probably this is better to improve the user experience :+1:

No, that's when it shows, but is actually caused by #1291.

Awesome, thanks a lot for investigating this! I'll fix the issue 💪

MangelMaxime commented 6 years ago

Because this PR, is about adding AppVeyor artifact usage I am merging it. Thank for adding it @alfonsogarciacaro should speed up startup time for people and allow smaller machine to work on this project like my laptop at work :)

I will list the Emit problem inside the #8 PR so we can have all the tasks in one place.

ncave commented 6 years ago

@alfonsogarciacaro The only place in FSAC I can find that uses FSharpChecker is here, and it has keepAssemblyContents = true.

Regardless, I don't think setting it to false does much besides throwing the AST away after it's generated (it still makes it, it has to). Perhaps you can elaborate a bit more what your optimization idea is.

alfonsogarciacaro commented 6 years ago

@ncave Awesome, after fixing #1291 the REPL is working perfectly now, thank you! :clap: :clap: :clap:

About the optimization, I was probably wrong. From the beginning I read in the FCS tutorial that the difference between getting the symbols vs the full AST was setting keepAssemblyContents = true, so I just assumed leaving it to false was a way to parse the code faster to only get the symbols. But it seems my assumption was not correct 😅

alfonsogarciacaro commented 6 years ago

BTW @StachuDotNet, you should be able to run the REPL now. Clone the feature/new_repl branch, run the build script and then yarn start :)

StachuDotNet commented 6 years ago

@alfonso, :D time for some documentation.

On Dec 12, 2017 5:46 AM, "Alfonso Garcia-Caro" notifications@github.com wrote:

BTW @StachuDotNet https://github.com/stachudotnet, you should be able to run the REPL now. Clone the feature/new_repl branch, run the build script and then yarn start :)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-repl/pull/10#issuecomment-351014716, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3VvpMrJgmJzGNg4KlTkCVjI587Ltzaks5s_lmGgaJpZM4QzzN7 .

ncave commented 6 years ago

@alfonsogarciacaro Awesome, thanks for fixing it.