Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .depend
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
src/reactjs_jsx_ppx.cmx : src/reactjs_jsx_ppx.cmi
src/reactjs_jsx_ppx.cmi :
src/res_ast_conversion.cmx : src/res_ast_conversion.cmi
src/res_ast_conversion.cmi :
src/res_ast_debugger.cmx : src/res_driver.cmx src/res_doc.cmx \
Expand All @@ -6,7 +8,7 @@ src/res_ast_debugger.cmi : src/res_driver.cmi
src/res_character_codes.cmx :
src/res_cli.cmx : src/res_driver_reason_binary.cmx \
src/res_driver_ml_parser.cmx src/res_driver_binary.cmx src/res_driver.cmx \
src/res_ast_debugger.cmx
src/res_ast_debugger.cmx src/reactjs_jsx_ppx.cmx
src/res_comment.cmx : src/res_comment.cmi
src/res_comment.cmi :
src/res_comments_table.cmx : src/res_parsetree_viewer.cmx src/res_doc.cmx \
Expand Down
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ depend:
$(OCAMLDEP) -native -I tests -I src src/*.ml src/*.mli tests/*.ml tests/*.mli > .depend

API_FILES = \
src/reactjs_jsx_ppx.cmx\
src/res_io.cmx\
src/res_minibuffer.cmx\
src/res_doc.cmx\
Expand Down Expand Up @@ -49,6 +50,8 @@ lib/rescript.exe: $(CLI_FILES)

build-native: lib/refmt.exe lib/rescript.exe depend

build-ppx: src/reactjs_jsx_ppx.cmx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does the ppx need a separate build-ppx rule? If it's a part "API_FILES", is this sufficient?


bootstrap: build-native
ocaml unix.cma ./scripts/bspack.ml -bs-main Res_cli -I src -o ./lib/rescript.ml
./lib/rescript.exe -parse ml -print ns ./lib/Rescript.ml > ./lib/Rescript2.ml
Expand Down Expand Up @@ -94,4 +97,4 @@ clean:
rm -rf lib/rescript.exe
rm -rf lib/test.exe
git clean -dfx src
.PHONY: clean test roundtrip-test termination dce exception reanalyze bootstrap build-native
.PHONY: clean test roundtrip-test termination dce exception reanalyze bootstrap build-native build-ppx
880 changes: 880 additions & 0 deletions src/reactjs_jsx_ppx.ml

Large diffs are not rendered by default.

39 changes: 39 additions & 0 deletions src/reactjs_jsx_ppx.mli
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
(*
This is the module that handles turning Reason JSX' agnostic function call into
a ReasonReact-specific function call. Aka, this is a macro, using OCaml's ppx
facilities; https://whitequark.org/blog/2014/04/16/a-guide-to-extension-
points-in-ocaml/
You wouldn't use this file directly; it's used by ReScript's
bsconfig.json. Specifically, there's a field called `react-jsx` inside the
field `reason`, which enables this ppx through some internal call in bsb
*)

(*
There are two different transforms that can be selected in this file (v2 and v3):
v2:
transform `[@JSX] div(~props1=a, ~props2=b, ~children=[foo, bar], ())` into
`ReactDOMRe.createElement("div", ~props={"props1": 1, "props2": b}, [|foo,
bar|])`.
transform `[@JSX] div(~props1=a, ~props2=b, ~children=foo, ())` into
`ReactDOMRe.createElementVariadic("div", ~props={"props1": 1, "props2": b}, foo)`.
transform the upper-cased case
`[@JSX] Foo.createElement(~key=a, ~ref=b, ~foo=bar, ~children=[], ())` into
`ReasonReact.element(~key=a, ~ref=b, Foo.make(~foo=bar, [||]))`
transform `[@JSX] [foo]` into
`ReactDOMRe.createElement(ReasonReact.fragment, [|foo|])`
v3:
transform `[@JSX] div(~props1=a, ~props2=b, ~children=[foo, bar], ())` into
`ReactDOMRe.createDOMElementVariadic("div", ReactDOMRe.domProps(~props1=1, ~props2=b), [|foo, bar|])`.
transform the upper-cased case
`[@JSX] Foo.createElement(~key=a, ~ref=b, ~foo=bar, ~children=[], ())` into
`React.createElement(Foo.make, Foo.makeProps(~key=a, ~ref=b, ~foo=bar, ()))`
transform the upper-cased case
`[@JSX] Foo.createElement(~foo=bar, ~children=[foo, bar], ())` into
`React.createElementVariadic(Foo.make, Foo.makeProps(~foo=bar, ~children=React.null, ()), [|foo, bar|])`
transform `[@JSX] [foo]` into
`ReactDOMRe.createElement(ReasonReact.fragment, [|foo|])`
*)

val rewrite_implementation : Parsetree.structure -> Parsetree.structure

val rewrite_signature : Parsetree.signature -> Parsetree.signature
28 changes: 23 additions & 5 deletions src/res_cli.ml
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ module ResClflags: sig
val files: string list ref
val interface: bool ref
val report: string ref
val ppx: string ref

val parse: unit -> unit
end = struct
Expand All @@ -178,6 +179,7 @@ end = struct
let origin = ref ""
let interface = ref false
let report = ref "pretty"
let ppx = ref "none"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we default this to the empty string ""?


let usage = "Usage:\n rescript <options> <file>\n\n" ^
"Examples:\n" ^
Expand All @@ -192,6 +194,7 @@ end = struct
("-print", Arg.String (fun txt -> print := txt), "Print either binary or ns. Default: ns");
("-width", Arg.Int (fun w -> width := w), "Specify the line length for the printer (formatter)");
("-interface", Arg.Unit (fun () -> interface := true), "Parse as interface");
("-ppx", Arg.String (fun txt -> ppx := txt), "Apply a specific built-in ppx before parsing, none or jsx. Default: none");
(* ("-report", Arg.String (fun txt -> report := txt), "Stylize errors and messages using color and context. Accepts `Pretty` and `Plain`. Default `Plain`") *)
]

Expand All @@ -201,7 +204,7 @@ end
module CliArgProcessor = struct
type backend = Parser: ('diagnostics) Res_driver.parsingEngine -> backend [@@unboxed]

let processFile ~isInterface ~width ~recover ~origin ~target ~report:_ filename =
let processFile ~isInterface ~width ~recover ~origin ~target ~report:_ ~ppx filename =
try
let len = String.length filename in
let processInterface =
Expand All @@ -227,6 +230,11 @@ module CliArgProcessor = struct
| _ -> false
in

let ppx = match ppx with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment down, this conversion doesn't add that much value?

| "jsx" -> `Jsx
| _ -> `None
in

let Parser backend = parsingEngine in
(* This is the whole purpose of the Color module above *)
Color.setup None;
Expand All @@ -243,8 +251,12 @@ module CliArgProcessor = struct
else exit 1
end
else
let parsetree = match ppx with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this a string and match on the string for now? Does this extra conversion give us something in the end?

| `Jsx -> Reactjs_jsx_ppx.rewrite_signature parseResult.parsetree
| `None -> parseResult.parsetree
in
printEngine.printInterface
~width ~filename ~comments:parseResult.comments parseResult.parsetree
~width ~filename ~comments:parseResult.comments parsetree
else
let parseResult = backend.parseImplementation ~forPrinter ~filename in
if parseResult.invalid then begin
Expand All @@ -258,19 +270,23 @@ module CliArgProcessor = struct
else exit 1
end
else
let parsetree = match ppx with
| `Jsx -> Reactjs_jsx_ppx.rewrite_implementation parseResult.parsetree
| `None -> parseResult.parsetree
in
printEngine.printImplementation
~width ~filename ~comments:parseResult.comments parseResult.parsetree
~width ~filename ~comments:parseResult.comments parsetree
with
| Failure txt ->
prerr_string txt;
prerr_newline();
exit 1
| _ -> exit 1
[@@raises exit]
[@@raises Invalid_argument, exit]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What raises Invalid_argument?

Copy link
Contributor Author

@jchavarri jchavarri Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing react ppx uses that exception to note cases with unexpected situations. I can count at least 19 different places, e.g.

raise
(Invalid_argument
"Key cannot be accessed inside of a component. Don't worry - you can always key a component from its \
parent!")

end


let [@raises exit] () =
let [@raises Invalid_argument, exit] () =
if not !Sys.interactive then begin
ResClflags.parse ();
match !ResClflags.files with
Expand All @@ -282,6 +298,7 @@ let [@raises exit] () =
~target:!ResClflags.print
~origin:!ResClflags.origin
~report:!ResClflags.report
~ppx:!ResClflags.ppx
""
| files ->
List.iter (fun filename ->
Expand All @@ -292,6 +309,7 @@ let [@raises exit] () =
~target:!ResClflags.print
~origin:!ResClflags.origin
~report:!ResClflags.report
~ppx:!ResClflags.ppx
filename
) files
end
6 changes: 6 additions & 0 deletions tests/api/resReactJsx.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// test React JSX file

@react.component
let make = (~msg) => {
<div> {msg->React.string} </div>
}
101 changes: 101 additions & 0 deletions tests/printer/reactJsx/__snapshots__/render.spec.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should make a separate folder for ppx tests: tests/ppx/...? What do you think?


exports[`commentAtTop.res 1`] = `
"@bs.obj
external makeProps: (~msg: 'msg, ~key: string=?, unit) => {\\"msg\\": 'msg} = \\"\\" // test React JSX file
let make =
(@warning(\\"-16\\") ~msg) => {
ReactDOMRe.createDOMElementVariadic(\\"div\\", [{msg->React.string}])
}
let make = {
let \\\\\\"CommentAtTop\\" = (\\\\\\"Props\\": {\\"msg\\": 'msg}) => make(~msg=\\\\\\"Props\\"[\\"msg\\"])
\\\\\\"CommentAtTop\\"
}
"
`;
exports[`externalWithCustomName.res 1`] = `
"module Foo = {
@bs.obj
external componentProps: (
~a: int,
~b: string,
~key: string=?,
unit,
) => {\\"a\\": int, \\"b\\": string} = \\"\\"
@bs.module(\\"Foo\\")
external component: React.componentLike<
{\\"a\\": int, \\"b\\": string},
React.element,
> = \\"component\\"
}
let t = React.createElement(
Foo.component,
Foo.componentProps(~a=1, ~b={\\"1\\"}, ()),
)
"
`;
exports[`innerModule.res 1`] = `
"module Bar = {
@bs.obj
external makeProps: (
~a: 'a,
~b: 'b,
~key: string=?,
unit,
) => {\\"a\\": 'a, \\"b\\": 'b} = \\"\\"
let make =
(@warning(\\"-16\\") ~a, @warning(\\"-16\\") ~b, _) => {
Js.log(\\"This function should be named \`InnerModule.react$Bar\`\\")
ReactDOMRe.createDOMElementVariadic(\\"div\\", [])
}
let make = {
let \\\\\\"InnerModule$Bar\\" = (\\\\\\"Props\\": {\\"a\\": 'a, \\"b\\": 'b}) =>
make(~b=\\\\\\"Props\\"[\\"b\\"], ~a=\\\\\\"Props\\"[\\"a\\"], ())
\\\\\\"InnerModule$Bar\\"
}
@bs.obj
external componentProps: (
~a: 'a,
~b: 'b,
~key: string=?,
unit,
) => {\\"a\\": 'a, \\"b\\": 'b} = \\"\\"
let component =
(@warning(\\"-16\\") ~a, @warning(\\"-16\\") ~b, _) => {
Js.log(\\"This function should be named \`InnerModule.react$Bar$component\`\\")
ReactDOMRe.createDOMElementVariadic(\\"div\\", [])
}
let component = {
let \\\\\\"InnerModule$Bar$component\\" = (\\\\\\"Props\\": {\\"a\\": 'a, \\"b\\": 'b}) =>
component(~b=\\\\\\"Props\\"[\\"b\\"], ~a=\\\\\\"Props\\"[\\"a\\"], ())
\\\\\\"InnerModule$Bar$component\\"
}
}
"
`;
exports[`topLevel.res 1`] = `
"@bs.obj
external makeProps: (
~a: 'a,
~b: 'b,
~key: string=?,
unit,
) => {\\"a\\": 'a, \\"b\\": 'b} = \\"\\"
let make =
(@warning(\\"-16\\") ~a, @warning(\\"-16\\") ~b, _) => {
Js.log(\\"This function should be named 'TopLevel.react'\\")
ReactDOMRe.createDOMElementVariadic(\\"div\\", [])
}
let make = {
let \\\\\\"TopLevel\\" = (\\\\\\"Props\\": {\\"a\\": 'a, \\"b\\": 'b}) =>
make(~b=\\\\\\"Props\\"[\\"b\\"], ~a=\\\\\\"Props\\"[\\"a\\"], ())
\\\\\\"TopLevel\\"
}
"
`;
6 changes: 6 additions & 0 deletions tests/printer/reactJsx/commentAtTop.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// test React JSX file

@react.component
let make = (~msg) => {
<div> {msg->React.string} </div>
}
6 changes: 6 additions & 0 deletions tests/printer/reactJsx/externalWithCustomName.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module Foo = {
@react.component @bs.module("Foo")
external component: (~a: int, ~b: string, _) => React.element = "component"
}

let t = <Foo.component a=1 b={"1"} />
16 changes: 16 additions & 0 deletions tests/printer/reactJsx/innerModule.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module Bar = {
@react.component
let make = (~a, ~b, _) => {
Js.log(
"This function should be named `InnerModule.react$Bar`",
)
<div />
}
@react.component
let component = (~a, ~b, _) => {
Js.log(
"This function should be named `InnerModule.react$Bar$component`",
)
<div />
}
}
1 change: 1 addition & 0 deletions tests/printer/reactJsx/render.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
runPrinter(__dirname, "jsx")
5 changes: 5 additions & 0 deletions tests/printer/reactJsx/topLevel.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@react.component
let make = (~a, ~b, _) => {
Js.log("This function should be named 'TopLevel.react'")
<div />
}
15 changes: 7 additions & 8 deletions tests/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ function parseNapkinStdinToNapkin(src, isInterface, width = 100) {
.stdout.toString("utf8");
}

function printFile(filename) {
function printFile(filename, ppx) {
let parserSrc;
switch (classifyLang(filename)) {
case "ocaml":
Expand All @@ -165,17 +165,15 @@ function printFile(filename) {
status: 0,
errorOutput: ""
};
break;


case "rescript":
default:
parserSrc = "res";
break;
}

let intf = isInterface(filename);

let args = ["-parse", parserSrc, "-print", "res", "-width", "80"];
let args = ["-parse", parserSrc, "-print", "res", "-width", "80", "-ppx", ppx];

if (intf) {
args.push("-interface");
Expand Down Expand Up @@ -204,15 +202,16 @@ let makeReproducibleFilename = (txt) => {
})
};

global.runPrinter = (dirname) => {
global.runPrinter = (dirname, ppx = "none") => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this the empty string or just keep it undefined as default value?

fs.readdirSync(dirname).forEach((base) => {
let filename = path.join(dirname, base);

if (!fs.lstatSync(filename).isFile() || base === "render.spec.js") {
return;
}

test(base, () => {
let {result, errorOutput, status} = printFile(filename);
let {result, errorOutput, status} = printFile(filename, ppx);
if (status > 0) {
let msg = `Test from file: ${filename} failed with error output:

Expand All @@ -226,7 +225,7 @@ Make sure the test input is syntactically valid.`;
expect(result).toMatchSnapshot();
}

if (process.env.ROUNDTRIP_TEST) {
if (process.env.ROUNDTRIP_TEST && ppx === "none") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here on why we don't run ppx'es in the roundtrip tests?

let intf = isInterface(filename);
let sexpAst = parseFileToSexp(filename);
let result2 = parseNapkinStdinToNapkin(result, intf, 80);
Expand Down