-
Notifications
You must be signed in to change notification settings - Fork 455
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
Playground 2022 #5375
Playground 2022 #5375
Conversation
@@ -69,6 +69,8 @@ val printer : (formatter -> t -> unit) ref | |||
val warning_printer : (t -> formatter -> Warnings.t -> unit) ref | |||
(** Hook for intercepting warnings. *) | |||
|
|||
val formatter_for_warnings : formatter ref |
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.
unfortunately i need this interface, otherwise I can't reset the warning buffer between multiple runs
@@ -37,7 +37,9 @@ o $SNAP/unstable/js_compiler.ml: bspack | ./bin/bspack.exe $LTO | |||
flags = -D BROWSER=true -MD -I ml $includes | |||
main = Jsoo_main | |||
|
|||
|
|||
o $SNAP/unstable/js_playground_compiler.ml: bspack | ./bin/bspack.exe $LTO |
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.
Not sure if there's a way to do conditional rules to add the playground snapshot build
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.
@cristianoc compilation performance should be generally good, so running this with every ninja build feels like the right thing to do. Also the jsoo_main
entrypoint is identical from a dependency perspective and always has been part of the build process ever since.
Note: The slowest part of building the playground is running jsoo with scripts/repl.js
, and we'd probably want to set up a CI job later to automate / test the bundle build process. We don't need to run this compilation on a regular basis since JSOO is quite reliable as long as the ocaml code compiles.
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.
here are some benchmarks.
Test instructions:
- Edit
jscomp/snapshot.ninja
to enable / disable certain rules. - Edit a line in
jscomp/ml/location.mli
- Run config / build
BASELINE: no snapshot rule for any playground
scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/reactDOMRe.cmi
rescript: [2/2] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build 10.22s user 4.53s system 291% cpu 5.057 total
TEST 1: snapshot rule for jsoo_main only
scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/unsafe_this.cmj
rescript: [3/3] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build 11.26s user 4.75s system 310% cpu 5.147 total
TEST 2: snapshot rule for jsoo_main and jsoo_playground_main
scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/flow_parser_reg_test.cmj
rescript: [4/4] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build 12.24s user 4.74s system 268% cpu 6.324 total
TEST 3: snapshot rule for jsoo_playground_main only
scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/flow_parser_reg_test.cmj
rescript: [3/3] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build 11.26s user 4.76s system 258% cpu 6.206 total
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.
Looks like the differences are absorbed by measurement noise, so nothing of note there.
1534ab2
to
a2b4a46
Compare
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.
Looks good to me. I think it can be merged unless there are other concerns.
CONTRIBUTING.md
Outdated
|
||
The entry point of the JSOO bundle is located in `jscomp/main/jsoo_main.ml` and the script for running JSOO can be found in `scripts/repl.js`. A full clean build can be done like this: | ||
The entry point of the JSOO bundle is located in `jscomp/main/jsoo_refmt_main.ml`, the code for packing the compiler into a single compiler file is located in `jscomp/snapshot.ninja`, and the script for running JSOO can be found in `scripts/repl.js`. A full clean build can be done like this: |
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.
Looks like json_refmt_main.ml
has been deleted in this diff. Is some of this file stale?
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.
ah yes I renamed that file bc refmt is gone. Will update.
For reference on the previous work in 2020, refer to #4518
Configure the parser to pass forPrinter:true when parsing / printing ReScript. This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc. Relevant bug reports: - rescript-lang/syntax#274 - rescript-lang/syntax#265
a2b4a46
to
e33b675
Compare
* Add all relevant changes from previous 2020 bundle API For reference on the previous work in 2020, refer to rescript-lang#4518 * Add ninja rule for building jsoo_refmt_main * Fix formatting edge-cases when formatting ReScript -> ReScript Configure the parser to pass forPrinter:true when parsing / printing ReScript. This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc. Relevant bug reports: - rescript-lang/syntax#274 - rescript-lang/syntax#265 * Introduce initial type hints in playground bundle results * Adapt playground to newest internal syntax apis * Make jsoo_refmt_main work again by removing refmt * Rename jsoo_refmt_main to jsoo_playground_main * Move jsoo_playground_main to jscomp/main * Remove irrelevant code * Remove Refmt_api * Make repl.js more reliable with missing .mli files * repl.js: Log target compiler file, set js_playground_main as default * Update CONTRIBUTING to align with the renamed playground jsoo entrypoint Co-authored-by: Patrick Stapfer <[email protected]>
Continuing the work of #5079. Here are the main changes:
Refmt_api
and somesrc/ast_406.ml
type mismatches)repl.js
file to allow compilation of different JS bundle versions (by default it will compile thejs_compile
bundle)Note on the
refmt
thing: I guess the only way to re-introduce refmt support would be via a separate bundle. Basically like in the good old BuckleScript playground times.The lack of refmt support eliminates half of the playground code though, so from a maintenance perspective it's way easier now.