Add BS JS Playground reason support #3976

ryyppy · 2019-11-19T15:53:23Z

This PR is an attempt to add refmt_api to the jsoo_main to be able to parse and interpret Reason code without independently compiling Reason to refmt.js and wire everything up ourselves.

I added CONTRIBUTION docs to describe the build workflow for creating bspacked refmt_api.ml and refmt_binary.ml files. The docs are only valid when reason#2495 is merged though.

The cool thing is that we could potentially have a properly synced up Reason version in the reason_api bundle, giving us something like this in the browser:

^ this is exciting, since it enables us to display this information in the Playground UI

There are still some open questions for this though (please help us out here @bobzhang):

~~Is this the right way to do it? Adding refmt_api will presumably cause the resulting exports.js file to grow. I have no perspective on how much the difference is to shipping refmt.js independently~~ -> See another comment below
~~Should we take the chance and refactor the public api (window.ocaml / window.reason) to something more unified such as window.bucklescript?~~ -> We will discuss this in another PR

all based on previous work of @thangngoc89

Sidenote:
[email protected] apparently has syntax regressions which need to be addressed before any further official releases!

thangngoc89

I left some comments, also there are several things that needs to be addressed:

there are unnecessary changes to bsb_native.ml and bsb.ml that makes this PR hard to review
refmt error catching needs to be implemented before merging
shake_compile doesn't work with reason

jscomp/main/jsoo_main.ml

CONTRIBUTING.md

ryyppy · 2019-11-20T07:04:50Z

lib/4.06.1/unstable/bsb_native.ml

        "\n\
         \n\
         let () = Js.log \"Hello, BuckleScript\"")]) ;
-    Dir (".vscode", [
-      File ("tasks.json",
+    Dir  (".vscode", [


No idea on what's causing this spaces... is it using some kind of formatter?

Huh, so it seems like the spaces are included in the jscomp/bsb/bsb_templates and ever since these files have never been checked in then?

I dropped the files from this PR, since it is not related

thangngoc89 · 2019-11-20T07:54:40Z

This looks good to me

ryyppy · 2019-11-20T08:18:35Z

An update on point 1) where I asked if it's the right way.
Now with further discussions, BS + refmt should be coupled to prevent users from switch & match different bs-platform & refmt versions on the playground. Goal is to be sure that code on the web is parsed the same way as a local installation.

To put everything in perspective number wise, here are some facts:

(Numbers are extracted by the day of this writing)

reasonml.github.io:

refmt.js (410kb gzipped / 2.6MB unzipped)
bsReasonReact.js, according to its size I suppose BS is included in there (930kb gzipped / 4.8MB unzipped)

nit.sketch.sh:

bs.js includes BS + refmt 5.3.0 (1.1MB gzipped / 5.3MB unzipped)

revery playground:

Revery_worker.js (1.7MB gzipped / 8.5MB unzipped)

Current build:

exports.js: 5.8MB without any uglification / compression at all

Conclusion:
Since we haven't even considered any bundle optimizations yet, I think we are in a good ballpark to make it smaller in the future

ryyppy · 2019-11-20T08:44:17Z

Basically ready for review, but Reason needs to merge the PR regarding bspacking refmt first to have the docs synced up with the actual Reason repo.

Next steps:

Make the external lib loading work (ReasonReact)
Improve the lib loading process by being able to point to different require paths (different React versions etc)
Improve the exposed API to be more concise

bobzhang · 2019-11-20T18:25:39Z

jscomp/main/jsoo_main.ml

+            "type" , inject @@ Js.string "error"
+      |]
+    )
+
 let () =  
  Bs_conditional_initial.setup_env ();


This looks good to me in general.
Can we make a separate file instead of enriching such file, like reason_main.ml, the reason is that
currently this is file is buildable, when reason linked in this file may not be easily buildable, we can have two files : one with vanilla ocaml which has less deps, the other has reason which is larger

Not sure what you mean by "when reason is linked, this file may not be easily buildable"? Do you mean because of the reliability of the Reason code?

But yes, we can totally do a "jsoo_reason_main.ml" file exposing the ocaml + reason interface as well

In general we don't compile packed files(to make compile time less for productivity) during dev but we compile jsoo_main.ml during dev. A separate file would help save some compilation time

@bobzhang So I would create a file jscomp/main/jsoo_refmt_main.ml. How would I configure ninja, or whatever part is responsible, to build this file the way you want it? I am not even sure how this is supposed to save compilation time. It looks like it is as fast as without reason?

@ryyppy just add a new file to main and run ./scripts/ninja.js config.

I think the best course of action would be jsoo_refmt_main.ml reusing functions from jsoo_main.ml instead of duplicating everything.

bobzhang · 2019-11-20T18:26:35Z

jscomp/main/jsoo_main.ml

@@ -180,7 +188,12 @@ let dir_directory d =
 let () =
  dir_directory "/static/cmis"

-let make_compiler name impl =
+module Converter = Refmt_api.Migrate_parsetree.Convert(Refmt_api.Migrate_parsetree.OCaml_404)(Refmt_api.Migrate_parsetree.OCaml_406)
+


Is reason developed against 404? Maybe a good time to bump into 406 as well

Should we open an issue on Reason to let them know that BS v7 and above won't have any active 4.02 builds anymore? Not sure what Reason planned to support on the OCaml side, but maybe they consider to raise the minimum requirement to 4.06?

yeah, if they raise to 406 then such conversion is free?

Apparently there seems to be a plan to upgrade. I created an issue to verify this:
reason#2496

ryyppy · 2019-11-22T16:17:19Z

I pushed some new changes adding the new file jsoo_refmt_main.ml, for now just copy / pasting the jsoo_main.ml since I want to verify first how ninja should be set up to build the file. Not sure if I am going the right direction here?

jscomp/main/jsoo_refmt_main.ml

ryyppy · 2019-11-23T20:59:02Z

@bobzhang Added a jscomp/main/jsoo_common.ml file for shared content between the two jsoo main entrypoints

I am a little bit hesitant to do bigger refactors since afaik we don't have any integration tests for the javascript bundle yet. I wanted to address tests in a later PR to make sure everything works in the future.

jchavarri · 2019-11-24T22:29:41Z

CONTRIBUTING.md

@@ -312,6 +312,53 @@ load the data. Right now we don't provide any instructions inside here yet, but
 [here's how the official ReasonML playground did
 it](https://github.com/reasonml/reasonml.github.io/blob/source/website/setupSomeArtifacts.js#L65).

+### Upgrading the Reason version


@ryyppy Thanks for this work documenting the process! One question / suggestion :)

I started reading this section as part of the larger "Contributing to the BS Playground Bundle" above, but got confused when 2 versions of refmt are discussed. My main doubt was "why does the BS playground need two versions of refmt? Wouldn't refmt_api be enough?".

Then I understood (maybe wrongly?) that refmt_api is what is used in the playground, and refmt_main3 is what BS uses when it runs natively, so it's not used in the playground (that was the confusing part).

If that assumption is right, maybe it'd be worth making the whole "Upgrading the Reason version" section an h2 (##) instead of h3, so that it "escapes" the "Contributing to the BS Playground Bundle" above and stands on its own, as an explanation on how these 2 packed files are generated, and how to keep Reason versions in sync with BS.

Maybe also rename the section to "Upgrading the Reason version for both native- and JavaScript-compiled versions of BS".

I hope that makes sense.

Good point, I lifted the "Reason upgrading" section to an H2 and made it kinda independent from the Playground header. I hope it is more clear now?

Also syncs reason_binary

This PR add specific error handling for refmt_api so it would show the correct location of the syntax error

This will restore the original `jsoo_main.ml` and introduce an enhanced `jsoo_refmt_main` with the `Refmt_Api` dependency built in. Also adds a rule to `jscomp/snapshot.ninja` to build bytecode for the new `jsoo_refmt_main.ml` file, so we can `repl.js` it into a bundle file. `repl.js` will build the new jsoo_refmt_main bytecode target instead.

…round

ryyppy · 2019-11-25T07:33:09Z

Rebased to newest master to be able to merge

ryyppy · 2019-11-26T09:27:19Z

Related to #3857

thangngoc89 suggested changes Nov 20, 2019

View reviewed changes

jscomp/main/jsoo_main.ml Outdated Show resolved Hide resolved

thangngoc89 reviewed Nov 20, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

thangngoc89 reviewed Nov 20, 2019

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

ryyppy commented Nov 20, 2019

View reviewed changes

ryyppy marked this pull request as ready for review November 20, 2019 08:33

bobzhang reviewed Nov 20, 2019

View reviewed changes

ryyppy mentioned this pull request Nov 21, 2019

Raise minimum OCaml AST support reasonml/reason#2496

Closed

bobzhang reviewed Nov 22, 2019

View reviewed changes

jscomp/main/jsoo_refmt_main.ml Outdated Show resolved Hide resolved

jscomp/main/jsoo_refmt_main.ml Outdated Show resolved Hide resolved

jscomp/main/jsoo_refmt_main.ml Outdated Show resolved Hide resolved

jchavarri reviewed Nov 24, 2019

View reviewed changes

ryyppy and others added 15 commits November 25, 2019 08:18

Add newest refmt_api to jsoo_main, add CONTRIBUTION docs

238db04

Also syncs reason_binary

Snapshot

27afbb9

Remove unrelated dir_directory change in jsoo_main

439e023

Fix shake_compile for reason syntax

cb14307

Undo changes to bsb.ml / bsb_native.ml

c67eed5

Snapshot js_compiler.ml

5135835

Small CONTRIBUTING changes

a2ebc76

Handle refmt error properly

569523a

This PR add specific error handling for refmt_api so it would show the correct location of the syntax error

Snapshot js_compiler.ml

e42819f

Remove obsolete playground building instructions from jscomp README

46f65ce

Snapshot js_compiler.ml / js_refmt_compiler.ml

a1c579f

Create jsoo_common.ml module for common logic for playground entrypoints

c2b4140

Snapshot js_compiler / js_refmt_compiler

5936cd8

CONTRIBUTING: Make Reason upgrade instructions independent from Playg…

611134b

…round

Snapshot js_compiler / js_reftm_compiler

4f9e376

ryyppy force-pushed the playground-reason-support branch from 6fc6b41 to 4f9e376 Compare November 25, 2019 07:30

bobzhang merged commit c500cfe into rescript-lang:master Nov 25, 2019

ryyppy mentioned this pull request Nov 26, 2019

update playground #3857

Closed

ryyppy deleted the playground-reason-support branch November 26, 2019 09:27

Add BS JS Playground reason support #3976

Add BS JS Playground reason support #3976

Uh oh!

Conversation

ryyppy commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thangngoc89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thangngoc89 commented Nov 20, 2019

Uh oh!

ryyppy commented Nov 20, 2019

Uh oh!

ryyppy commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryyppy Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryyppy commented Nov 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryyppy commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryyppy commented Nov 25, 2019

Uh oh!

ryyppy commented Nov 26, 2019

Uh oh!

Uh oh!

ryyppy commented Nov 19, 2019 •

edited

Loading

thangngoc89 left a comment •

edited

Loading

ryyppy Nov 20, 2019 •

edited

Loading

ryyppy commented Nov 23, 2019 •

edited

Loading