get ppx working in sandbox with small ninja issue #3516
Conversation
| This looks good to me, but can you make it stand-alone since make them together may affect the code size of vanilla ocaml playground |
| btw, it seems we can integrate |
| @@ -0,0 +1,1092 @@ | |||
| (* | |||
There was a problem hiding this comment.
Is this the right location for this file?
jscomp/main/jsoo_main.ml Outdated
| try | ||
| impl (Lexing.from_string | ||
| (if prefix then "[@@@bs.config{no_export}]\n#1 \"repl.ml\"\n" ^ str else str )) | ||
| |> Reactjs_jsx_ppx_v3.rewrite |
There was a problem hiding this comment.
Would you like this to only be run conditionally?
There was a problem hiding this comment.
Currently the playground on the Reason site only compiles conditionally. Are there plans to change it to always compile? Reason JSX vs ReasonReact JSX might be something to consider.
There was a problem hiding this comment.
Yeah talking to Bob offline and we will probably compile two different versions - one with this built in.
jscomp/syntax/reactjs_jsx_ppx_v3.ml Outdated
| | [] -> None | ||
| | x :: l -> if p x then Some x else find_opt p l | ||
| | ||
| #if OCAML_VERSION =~ ">4.03.0" then |
There was a problem hiding this comment.
Yeah this makes long term support much easier and will avoid a lot of needless copying if we can compile this file to all conditions.
| Location.input_name := "Toplevel"; | ||
| mapper.structure mapper code | ||
| #else | ||
| let () = Ast_mapper.register "JSX" (fun _argv -> jsxMapper ()) |
There was a problem hiding this comment.
node ./scripts/ninja.js && ninja -C jscomp fails when this is written this way, even though this is "correct", but repl.js seems to set BS_COMPILER_IN_BROWSER and work properly. I think the env isn't set properly in ninja -C jscomp - any ideas?
| | {loc; txt = Ldot (modulePath, ("createElement" | "make"))} -> | ||
| (match !jsxVersion with | ||
| | Some 2 -> transformUppercaseCall modulePath mapper loc attrs callExpression callArguments | ||
| | None |
There was a problem hiding this comment.
v2 vs v3 should actually be another cppo statement here and a few lines below. Is there a good/easy way to add another conditional variable? That way we can fully replace the other copied files.
yeah we can do that if you'd prefer - we need to be able to conditionally flip between 2 and 3 though. |
What's the best way to do this? Compile it as a separate entry point? Is there an example of how they would call each other? Would it need to be composed in JS? |
| e(`hash camlp4 2>/dev/null || { echo >&2 "camlp4 not installed. Please install: opam install camlp4"; exit 1; }`) | ||
| // e(`hash camlp4 2>/dev/null || { echo >&2 "camlp4 not installed. Please install: opam install camlp4"; exit 1; }`) | ||
| | ||
| // require('../scripts/release').run() |
| build ../lib/reactjs_jsx_ppx_2.exe: link stubs/stubs.cmxa ext/ext.cmxa common/common.cmxa syntax/reactjs_jsx_ppx_v2.cmx | ||
| libs = ocamlcommon.cmxa | ||
| build ../lib/reactjs_jsx_ppx_js.exe: link stubs/stubs.cmxa ext/ext.cmxa common/common.cmxa syntax/reactjs_jsx_ppx_js.cmx | ||
| libs = ocamlcommon.cmxa |
There was a problem hiding this comment.
it does not make sense to build reactjs_jsx_ppx_js.exe, right?
| Closing in favor of #3523 |
3 participants
This gets the ppx working in the try page on reasonml.github.io and also sets us up for cppo and the ability to delete the other files. Need some small guidance on how to do that - will post questions. Fixes #3512