Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
8066745
Format files with OCamlformat 0.27.0
johnhaley81 Aug 22, 2025
b3f1f52
Add dataAttrs support for React elements
johnhaley81 Aug 21, 2025
740158f
Apply OCamlformat 0.27.0 formatting to feature branch
johnhaley81 Aug 22, 2025
916cd1f
Add project documentation for Claude Code
johnhaley81 Aug 23, 2025
bf982de
Implement zero-runtime data attributes support in PPX
johnhaley81 Aug 26, 2025
db9ead6
Update data attributes documentation for new JSX support
johnhaley81 Aug 26, 2025
a2a9f48
Remove redundant comments that describe what code does
johnhaley81 Aug 26, 2025
a467adf
Fix data attributes build and update test snapshots
johnhaley81 Aug 26, 2025
d42a9a5
Fix data attributes PPX bugs and restore working tests
johnhaley81 Aug 26, 2025
0470056
Add PPX deduplication fix
johnhaley81 Aug 27, 2025
3bbc1a6
Fix PPX external declaration deduplication for JSX data attributes
johnhaley81 Aug 27, 2025
b89c750
Remove temporary data attributes test file
johnhaley81 Aug 28, 2025
f8e2288
Add .serena/ to .gitignore
johnhaley81 Aug 28, 2025
4ca0aa3
Remove project-specific Claude instructions file
johnhaley81 Aug 29, 2025
2a41068
Fix critical PPX bug: DOM element externals not injected at structure…
johnhaley81 Sep 1, 2025
b37645d
Add comprehensive PPX fix analysis documentation
johnhaley81 Sep 1, 2025
cfd5e5f
Complete PPX data attributes fix with comprehensive test validation
johnhaley81 Sep 1, 2025
1cf9af5
Clean up codebase: remove analysis docs and comments
johnhaley81 Sep 2, 2025
4edc014
Fix PPX data attributes bug and update test patterns
johnhaley81 Sep 2, 2025
5fd0014
Streamline data attributes demo to showcase practical use cases
johnhaley81 Sep 2, 2025
ede06ea
Add comprehensive tests for data attributes with dynamic values
johnhaley81 Sep 3, 2025
7f7ab19
Fix getAttribute external binding for test runtime compatibility
johnhaley81 Sep 3, 2025
6a6f72e
Refactor test suite: consolidate data attributes tests and add warnin…
johnhaley81 Sep 3, 2025
7942e3e
Remove redundant React integration test file
johnhaley81 Sep 3, 2025
de819ed
Fix PPX test configuration: revert hardcoded paths to use opam binaries
johnhaley81 Sep 3, 2025
9d8d6d8
Format codebase with ocamlformat
johnhaley81 Sep 3, 2025
e61aa5a
Improve PPX comment clarity for domProps fallback logic
johnhaley81 Sep 3, 2025
3c7d5dc
Add Merlin hiding to generated data attribute externals and calls
johnhaley81 Sep 3, 2025
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
2 changes: 1 addition & 1 deletion .ocamlformat
Original file line number Diff line number Diff line change
@@ -1 +1 @@
version = 0.26.0
version = 0.27.0
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't change the ocamlformat version in a PR that adds a feature

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 had that split out to #900, if that PR is good then this change will be removed from this PR when that other is added. I don't know how to do a merge chain from multiple PRs from a fork :(

19 changes: 19 additions & 0 deletions demo/main.re
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,24 @@ module WithoutForward = {
};
};

module DataAttrsDemo = {
[@react.component]
let make = () => {
let dataAttrs =
[("testid", "demo-element"), ("component", "DataAttrsDemo")]
|> Js.Dict.fromList;

<section>
<h3> {React.string("DataAttrs Demo")} </h3>
<div dataAttrs>
{React.string(
"This div has data-testid='demo-element' and data-component='DataAttrsDemo'",
)}
</div>
</section>;
};
};

module App = {
[@react.component]
let make = (~initialValue) => {
Expand All @@ -237,6 +255,7 @@ module App = {
<UseReducerNoProblemo key="use-reducer-no-problemo" />
<FragmentsEverywhere key="fragments-everywhere" />
<WithoutForward key="without-forward" />
<DataAttrsDemo key="data-attrs-demo" />
</main>;
};
};
Expand Down
47 changes: 27 additions & 20 deletions ppx/reason_react_ppx.ml
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ let constantString ~loc str =

let safeTypeFromValue valueStr =
match getLabel valueStr with
| Some valueStr when String.sub valueStr 0 1 = "_" -> ("T" ^ valueStr)
| Some valueStr when String.sub valueStr 0 1 = "_" -> "T" ^ valueStr
| Some valueStr -> valueStr
| None -> ""

Expand Down Expand Up @@ -229,8 +229,10 @@ let hasAttrOnBinding { pvb_attributes; _ } =
let getFnName binding =
match binding with
| { pvb_pat = { ppat_desc = Ppat_var { txt; _ }; _ }; _ } -> txt
| { pvb_loc; _} ->
Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead."
| { pvb_loc; _ } ->
Location.raise_errorf ~loc:pvb_loc
"[@react.component] cannot be used with a destructured binding. Please \
use it on a `let make = ...` binding instead."

let makeNewBinding binding expression newName =
match binding with
Expand All @@ -243,7 +245,9 @@ let makeNewBinding binding expression newName =
pvb_attributes = [ merlinFocus ];
}
| { pvb_loc; _ } ->
Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead."
Location.raise_errorf ~loc:pvb_loc
"[@react.component] cannot be used with a destructured binding. Please \
use it on a `let make = ...` binding instead."

(* Lookup the value of `props` otherwise raise errorf *)
let getPropsNameValue _acc (loc, expr) =
Expand All @@ -252,7 +256,9 @@ let getPropsNameValue _acc (loc, expr) =
{ pexp_desc = Pexp_ident { txt = Lident str; _ }; _ } ) ->
{ propsName = str }
| { txt; loc }, _ ->
Location.raise_errorf ~loc "[@react.component] only accepts 'props' as a field, given: %s" (Longident.last_exn txt)
Location.raise_errorf ~loc
"[@react.component] only accepts 'props' as a field, given: %s"
(Longident.last_exn txt)

(* Lookup the `props` record or string as part of [@react.component] and store
the name for use when rewriting *)
Expand All @@ -261,22 +267,22 @@ let getPropsAttr payload =
match payload with
| Some
(PStr
({
pstr_desc =
Pstr_eval ({ pexp_desc = Pexp_record (recordFields, None); _ }, _);
_;
}
:: _rest)) ->
({
pstr_desc =
Pstr_eval ({ pexp_desc = Pexp_record (recordFields, None); _ }, _);
_;
}
:: _rest)) ->
List.fold_left getPropsNameValue defaultProps recordFields
| Some
(PStr
({
pstr_desc =
Pstr_eval
({ pexp_desc = Pexp_ident { txt = Lident "props"; _ }; _ }, _);
_;
}
:: _rest)) ->
({
pstr_desc =
Pstr_eval
({ pexp_desc = Pexp_ident { txt = Lident "props"; _ }; _ }, _);
_;
}
:: _rest)) ->
{ propsName = "props" }
| Some (PStr ({ pstr_desc = Pstr_eval (_, _); pstr_loc; _ } :: _rest)) ->
Location.raise_errorf ~loc:pstr_loc
Expand Down Expand Up @@ -487,7 +493,7 @@ let jsxExprAndChildren ~component_type ~loc ~ctxt mapper ~keyProps children =
children *)
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxs") },
None,
Some (Binding.React.array ~loc children))
Some (Binding.React.array ~loc children) )
| None, (label, key) :: _ ->
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxKeyed") },
Some (label, key),
Expand Down Expand Up @@ -645,7 +651,8 @@ let jsxMapper =
match expr.pexp_desc with
| Pexp_fun (Labelled "key", _, _, _) | Pexp_fun (Optional "key", _, _, _) ->
Location.raise_errorf ~loc:expr.pexp_loc
("~key cannot be accessed from the component props. Please set the key where the component is being used.")
"~key cannot be accessed from the component props. Please set the \
key where the component is being used."
| Pexp_fun
( ((Optional label | Labelled label) as arg),
default,
Expand Down
3 changes: 2 additions & 1 deletion ppx/test/react.t
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Demonstrate how to use the React JSX PPX
'use strict';

const Belt__Belt_List = require("melange.belt/belt_List.js");
const ReactDOM = require("reason-react/ReactDOM.js");
const JsxRuntime = require("react/jsx-runtime");

function X$App(Props) {
Expand All @@ -46,7 +47,7 @@ Demonstrate how to use the React JSX PPX
tl: /* [] */ 0
}
}, (function (greeting) {
return JsxRuntime.jsx("h1", {
return ReactDOM.jsx("h1", {
children: greeting
});
})));
Expand Down
122 changes: 95 additions & 27 deletions src/ReactDOM.re
Original file line number Diff line number Diff line change
Expand Up @@ -484,30 +484,35 @@ module Experimental = {
external preloadOptions:
(
~_as: [
| `audio
| `document
| `embed
| `fetch
| `font
| `image
| [@mel.as "object"] `object_
| `script
| `style
| `track
| `video
| `worker
],
~fetchPriority: [ | `auto | `high | `low]=?,
~referrerPolicy: [
| [@mel.as "no-referrer"] `noReferrer
| [@mel.as "no-referrer-when-downgrade"]
`noReferrerWhenDowngrade
| [@mel.as "origin"] `origin
| [@mel.as "origin-when-cross-origin"]
`originWhenCrossOrigin
| [@mel.as "unsafe-url"] `unsafeUrl
]
=?,
| `audio
| `document
| `embed
| `fetch
| `font
| `image
| [@mel.as "object"] `object_
| `script
| `style
| `track
| `video
| `worker
],
~fetchPriority:
[
| `auto
| `high
| `low
]
=?,
~referrerPolicy:
[
| [@mel.as "no-referrer"] `noReferrer
| [@mel.as "no-referrer-when-downgrade"] `noReferrerWhenDowngrade
| [@mel.as "origin"] `origin
| [@mel.as "origin-when-cross-origin"] `originWhenCrossOrigin
| [@mel.as "unsafe-url"] `unsafeUrl
]
=?,
~imageSrcSet: string=?,
~imageSizes: string=?,
~crossOrigin: string=?,
Expand All @@ -520,11 +525,29 @@ module Experimental = {
[@deriving jsProperties]
type preinitOptions = {
[@mel.as "as"]
_as: [ | `script | `style],
_as: [
| `script
| `style
],
[@mel.optional]
fetchPriority: option([ | `auto | `high | `low]),
fetchPriority:
option(
[
| `auto
| `high
| `low
],
),
[@mel.optional]
precedence: option([ | `reset | `low | `medium | `high]),
precedence:
option(
[
| `reset
| `low
| `medium
| `high
],
),
[@mel.optional]
crossOrigin: option(string),
[@mel.optional]
Expand Down Expand Up @@ -1609,6 +1632,9 @@ type domProps = {
suppressContentEditableWarning: option(bool),
[@mel.optional]
suppressHydrationWarning: option(bool),
/* data attributes */
[@mel.optional]
dataAttrs: option(Js.Dict.t(string)),
Copy link
Member

Choose a reason for hiding this comment

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

dataAttrs isn't a proper name imo. The HTML name feels right for it "dataset" (per https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset)

I like the idea of Js.dict instead of Js.t

Copy link
Member

Choose a reason for hiding this comment

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

Because the dataset prop isn't supported in React, this would need to be a ppx transformation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted this to a PPX transformation

};

// As we've removed `ReactDOMRe.createElement`, this enables patterns like
Expand All @@ -1625,16 +1651,58 @@ external createDOMElementVariadic:
(string, ~props: domProps=?, array(React.element)) => React.element =
"createElement";

// Helper function to process dataAttrs
let processDataAttrs = (props: domProps): domProps => {
Copy link
Member

Choose a reason for hiding this comment

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

Aside from the runtime cost, this implementation contains too much of everything. 3 Obj.magics in 30 lines:

  1. to cast props into to Js.Dict.t
  2. because some casting has been made, dataAttrs needs to be treated as Js.Dict.t again
  3. to cast Js.Dict.t back to props

also, iterating over all props when there's data-* feels mega wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this is removed now

switch (props.dataAttrs) {
| None => props // Short circuit: if no data attributes provided, return props unchanged for better performance
| Some(_) =>
props
|> Obj.magic
|> Js.Dict.entries
|> Js.Array.reduce(
~f=
acc =>
fun
| ("dataAttrs", dataAttrsDict) =>
dataAttrsDict
|> Obj.magic
|> Js.Dict.entries
|> Js.Array.reduce(
~f=
(acc, (dataKey, dataValue)) =>
[
("data-" ++ dataKey, dataValue |> Obj.magic: string),
...acc,
],
~init=acc,
)
| (key, value) => [(key, value), ...acc],
~init=[],
)
|> Js.Dict.fromList
|> Obj.magic
};
};

// JSX functions with dataAttrs processing
[@mel.module "react/jsx-runtime"]
external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.element =
"jsx";
let jsxKeyed = (component: string, props: domProps, ~key=?, ()) =>
jsxKeyed(component, processDataAttrs(props), ~key?, ());
Copy link
Member

Choose a reason for hiding this comment

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

As ReactDOM tries not to have any runtime, this application is undesired

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrated to a PPX transformation instead of a runtime one


[@mel.module "react/jsx-runtime"]
external jsx: (string, domProps) => React.element = "jsx";
let jsx = (component: string, props: domProps) =>
jsx(component, processDataAttrs(props));

[@mel.module "react/jsx-runtime"]
external jsxs: (string, domProps) => React.element = "jsxs";
let jsxs = (component: string, props: domProps) =>
jsxs(component, processDataAttrs(props));

[@mel.module "react/jsx-runtime"]
external jsxsKeyed: (string, domProps, ~key: string=?, unit) => React.element =
"jsxs";
let jsxsKeyed = (component: string, props: domProps, ~key=?, ()) =>
jsxsKeyed(component, processDataAttrs(props), ~key?, ());
Loading
Loading