Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

jsx: allow locally abstract types and type constraints on @react.component definitions #487

Merged
merged 2 commits into from
May 7, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
11 changes: 8 additions & 3 deletions src/reactjs_jsx_ppx_v3.ml
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,10 @@ let otherAttrsPure (loc, _) = loc.txt <> "react.component"
let hasAttrOnBinding { pvb_attributes } = find_opt hasAttr pvb_attributes <> None

(* Finds the name of the variable the binding is assigned to, otherwise raises Invalid_argument *)
let getFnName binding =
let rec getFnName binding =
match binding with
| { pvb_pat = { ppat_desc = Ppat_var { txt } } } -> txt
| { ppat_desc = Ppat_var { txt } } -> txt
| { ppat_desc = Ppat_constraint (pat, _) } -> getFnName pat
| _ -> raise (Invalid_argument "react.component calls cannot be destructured.")
[@@raises Invalid_argument]

Expand Down Expand Up @@ -487,7 +488,7 @@ let jsxMapper () =
let bindingLoc = binding.pvb_loc in
let bindingPatLoc = binding.pvb_pat.ppat_loc in
let binding = { binding with pvb_pat = { binding.pvb_pat with ppat_loc = emptyLoc }; pvb_loc = emptyLoc } in
let fnName = getFnName binding in
let fnName = getFnName binding.pvb_pat in
let internalFnName = fnName ^ "$Internal" in
let fullModuleName = makeModuleName fileName !nestedModules fnName in
let modifiedBindingOld binding =
Expand All @@ -506,6 +507,10 @@ let jsxMapper () =
spelunkForFunExpression innerFunctionExpression
| { pexp_desc = Pexp_sequence (_wrapperExpression, innerFunctionExpression) } ->
spelunkForFunExpression innerFunctionExpression
| { pexp_desc = Pexp_newtype (_label, innerFunctionExpression) } ->
spelunkForFunExpression innerFunctionExpression
| { pexp_desc = Pexp_constraint (innerFunctionExpression, _typ) } ->
spelunkForFunExpression innerFunctionExpression
| _ ->
raise
(Invalid_argument
Expand Down
6 changes: 6 additions & 0 deletions tests/ppx/react/expected/newtype.res.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
@obj external makeProps: (~a: a, ~b: 'b, ~key: string=?, unit) => {"a": a, "b": 'b} = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Note this won't compile: type a.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the transform is incorrect. Will revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

@glennsl reverted the PR for now, we'll need a new PR with a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap. Shouldn't have assumed the tests were run through the compiler. I'll fix. I suspect this will make it a much more invasive change though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also have a concrete piece of UI code to verify the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can certainly make one. Is there anywhere I can put this, or do you just want something to test manually on the side?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a gist is ok, we don't have "full stack integration tests", so manual it is.

let make = (type a, ~a: a, ~b, _) => ReactDOMRe.createDOMElementVariadic("div", [])
let make = {
let \"Newtype" = (\"Props": {"a": a, "b": 'b}) => make(~b=\"Props"["b"], ~a=\"Props"["a"])
\"Newtype"
}
8 changes: 8 additions & 0 deletions tests/ppx/react/expected/typeConstraint.res.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@obj external makeProps: (~a: 'a, ~b: 'b, ~key: string=?, unit) => {"a": 'a, "b": 'b} = ""
let make:
type a. (~a: a, ~b: a, a) => React.element =
(~a, ~b, _) => ReactDOMRe.createDOMElementVariadic("div", [])
let make = {
let \"TypeConstraint" = (\"Props": {"a": 'a, "b": 'b}) => make(~b=\"Props"["b"], ~a=\"Props"["a"])
\"TypeConstraint"
}
2 changes: 2 additions & 0 deletions tests/ppx/react/newtype.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@react.component
let make = (type a, ~a: a, ~b, _) => <div />
2 changes: 2 additions & 0 deletions tests/ppx/react/typeConstraint.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@react.component
let make: type a. (~a: a, ~b: a, a) => React.element = (~a, ~b, _) => <div />