diff --git a/.gitignore b/.gitignore index 05cd7e269..28b4b00dd 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,4 @@ _build # Editor /.idea/ _opam +.serena/ diff --git a/.ocamlformat b/.ocamlformat index ed7d4b31d..9ed4c2620 100644 --- a/.ocamlformat +++ b/.ocamlformat @@ -1 +1 @@ -version = 0.26.0 +version = 0.27.0 diff --git a/demo/main.re b/demo/main.re index 3edee04fb..aa5bb4232 100644 --- a/demo/main.re +++ b/demo/main.re @@ -221,6 +221,83 @@ module WithoutForward = { }; }; +module DataAttrsDemo = { + [@react.component] + let make = () => { +
+

{React.string("Data Attributes Demo")}

+

+ {React.string( + "Demonstrating common use cases for HTML data attributes with ReasonReact", + )} +

+
+

{React.string("Testing & QA")}

+ + +
+
+

{React.string("Analytics & Tracking")}

+ + + {React.string("External Documentation")} + +
+
+

{React.string("Component State")}

+
+ {React.string("High Priority Task")} +
+ + {React.string("Notifications")} + +
+
+

{React.string("Accessibility Support")}

+ +
+ {React.string("Please fix validation errors")} +
+
+
+

{React.string("Complex Example")}

+

+ {React.string( + "Multiple data attributes working together for testing, analytics, theming, and state management.", + )} +

+
+
; + }; +}; + module App = { [@react.component] let make = (~initialValue) => { @@ -237,6 +314,7 @@ module App = { + ; }; }; diff --git a/docs/adding-data-props.md b/docs/adding-data-props.md index da0fc8fcb..f8fbe3819 100644 --- a/docs/adding-data-props.md +++ b/docs/adding-data-props.md @@ -2,21 +2,88 @@ title: Adding data-* attributes --- -Reason doesn't support using props with dashes right now, ie: `data-id` or `data-whatever`. You can overcome this by creating a `Spread` component: +ReasonReact now supports data attributes directly in JSX with zero runtime overhead. Data attributes are transformed at compile-time for optimal performance. + +## Direct JSX Support + +You can now use `data_*` attributes directly in JSX: ```reason -/* Spread.re */ [@react.component] -let make = (~props, ~children) => React.cloneElement(children, props); +let make = () => { +
+ {React.string("Hello World")} +
+}; +``` + +This compiles to efficient code with proper `data-*` attribute names in the DOM: +```html +
+ Hello World +
``` -Using Spread: +## How It Works + +The PPX automatically detects `data_*` attributes and: +1. **Transforms names**: `data_testid` becomes `data-testid` in the DOM +2. **Zero runtime overhead**: All transformation happens at compile-time +3. **Backwards compatible**: Elements without data attributes use the standard path + +## Examples +### Basic Usage ```reason -[@react.component] -let make = () => - - /* This div will now have the `data-cy` attribute in the DOM! */ -
- ; +// Single data attribute + + +// Multiple data attributes +
+ {React.string("User content")} +
+``` + +### Combined with Other Props +```reason + +``` + +### Backwards Compatibility +```reason +// Elements without data attributes work exactly as before +
+ {React.string("No data attributes")} +
+``` + +## Migration from Old Workarounds + +If you were using the `Spread` component workaround: + +### Before (Old Workaround) +```reason + +
+ +``` + +### After (Direct JSX) +```reason +
``` + +## Technical Details + +- **Compile-time transformation**: No runtime performance impact +- **DOM elements only**: Works with `div`, `span`, `button`, etc. +- **Automatic naming**: `data_testid` → `data-testid`, `data_user_id` → `data-user-id` +- **Type safe**: Compile-time validation of attribute syntax diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index f37c7be83..7c302dd21 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -45,16 +45,163 @@ let merlinFocus = let nolabel = Nolabel let labelled str = Labelled str let optional str = Optional str +let externalDeclarations = ref [] + +let externalExists name declarations = + List.exists + (fun decl -> + match decl.pstr_desc with + | Pstr_primitive { pval_name; _ } -> pval_name.txt = name + | _ -> false) + declarations + +let getLabelOrEmpty label = + match label with Optional str | Labelled str -> str | Nolabel -> "" + +let isDataProp label = + let labelStr = getLabelOrEmpty label in + String.length labelStr >= 5 && String.sub labelStr 0 5 = "data_" + +let transformToKebabCase name = + if String.length name > 5 && String.sub name 0 5 = "data_" then + let suffix = String.sub name 5 (String.length name - 5) in + let kebabSuffix = String.map (function '_' -> '-' | c -> c) suffix in + "data-" ^ kebabSuffix + else name + +let generateExternalName ~elementName ~props = + let propNames = List.map (fun (label, _) -> getLabelOrEmpty label) props in + let propString = String.concat "_" propNames in + let hash = Digest.to_hex (Digest.string propString) in + Printf.sprintf "makeProps_%s_%s" elementName (String.sub hash 0 8) + +let createMelObjAttribute ~loc = + { + attr_name = { txt = "mel.obj"; loc }; + attr_payload = PStr []; + attr_loc = loc; + } + +let createMelAsAttribute ~loc jsName = + { + attr_name = { txt = "mel.as"; loc }; + attr_payload = + PStr + [ + Builder.pstr_eval ~loc + (Builder.pexp_constant ~loc (Pconst_string (jsName, loc, None))) + []; + ]; + attr_loc = loc; + } + +let createWarningSuppressionAttribute ~loc = + { + attr_name = { txt = "warning"; loc }; + attr_payload = + PStr + [ + Builder.pstr_eval ~loc + (Builder.pexp_constant ~loc (Pconst_string ("-32", loc, None))) + []; + ]; + attr_loc = loc; + } + +let rec buildArrowType ~loc props = + match props with + | [] -> + Builder.ptyp_arrow ~loc Nolabel + (Builder.ptyp_constr ~loc { txt = Lident "unit"; loc } []) + (Builder.ptyp_var ~loc "a") + | (label, _) :: rest -> + let propName = getLabelOrEmpty label in + let propType = + if propName = "children" then + Builder.ptyp_constr ~loc + { txt = Ldot (Lident "React", "element"); loc } + [] + else if propName = "style" then + Builder.ptyp_constr ~loc + { txt = Ldot (Ldot (Lident "ReactDOM", "Style"), "t"); loc } + [] + else if propName = "onClick" then + Builder.ptyp_arrow ~loc Nolabel + (Builder.ptyp_var ~loc "event") + (Builder.ptyp_constr ~loc { txt = Lident "unit"; loc } []) + else if propName = "onChange" then + Builder.ptyp_arrow ~loc Nolabel + (Builder.ptyp_var ~loc "event") + (Builder.ptyp_constr ~loc { txt = Lident "unit"; loc } []) + else Builder.ptyp_constr ~loc { txt = Lident "string"; loc } [] + in + let finalLabel, propType' = + if isDataProp label then + let jsName = transformToKebabCase propName in + let melAsAttr = createMelAsAttribute ~loc jsName in + (Labelled propName, { propType with ptyp_attributes = [ melAsAttr ] }) + else (Labelled propName, propType) + in + Builder.ptyp_arrow ~loc finalLabel propType' (buildArrowType ~loc rest) + +let createExternalDeclaration ~name ~props ~loc = + { + pstr_desc = + Pstr_primitive + { + pval_name = { txt = name; loc }; + pval_type = buildArrowType ~loc props; + pval_prim = [ "" ]; + (* Empty string for [@mel.obj] *) + pval_attributes = + [ + createMelObjAttribute ~loc; + createWarningSuppressionAttribute ~loc; + List.hd merlinHideAttrs; + ]; + pval_loc = loc; + }; + pstr_loc = loc; + } module Binding = struct (* Binding is the interface that the ppx relies on to interact with the react bindings. Here we define the same APIs as the bindings but it generates Parsetree nodes *) module ReactDOM = struct - let domProps ~applyLoc ~loc props = - Builder.pexp_apply ~loc:applyLoc - (Builder.pexp_ident ~loc:applyLoc ~attrs:merlinHideAttrs - { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) - props + let domProps ~applyLoc ~loc ?(elementName = "element") props = + let hasDataAttrs = + List.exists (fun (label, _) -> isDataProp label) props + in + + if hasDataAttrs then ( + let externalName = generateExternalName ~elementName ~props in + + (* Create external declaration only with labeled props ([@mel.obj] adds unit automatically) *) + let labeledProps = + List.filter + (fun (label, _) -> match label with Nolabel -> false | _ -> true) + props + in + let externalDecl = + createExternalDeclaration ~name:externalName ~props:labeledProps ~loc + in + (* Only add external if it doesn't already exist to prevent duplicates *) + if not (externalExists externalName !externalDeclarations) then + externalDeclarations := externalDecl :: !externalDeclarations; + Builder.pexp_apply ~loc + (Builder.pexp_ident ~loc ~attrs:merlinHideAttrs + { txt = Lident externalName; loc }) + (labeledProps + @ [ + ( Nolabel, + Builder.pexp_construct ~loc { txt = Lident "()"; loc } None ); + ])) + else + (* Use standard domProps if we don't have to inject data attrs *) + Builder.pexp_apply ~loc:applyLoc + (Builder.pexp_ident ~loc:applyLoc ~attrs:merlinHideAttrs + { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) + props end module React = struct @@ -79,7 +226,7 @@ module Binding = struct [ (nolabel, fragment); ( nolabel, - ReactDOM.domProps ~applyLoc:loc ~loc + ReactDOM.domProps ~applyLoc:loc ~loc ~elementName:"fragment" [ (labelled "children", children); (nolabel, Builder.unit) ] ); ] @@ -108,7 +255,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 -> "" @@ -229,8 +376,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 @@ -243,7 +392,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) = @@ -252,7 +403,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 *) @@ -261,22 +414,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 @@ -487,7 +640,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), @@ -613,7 +766,8 @@ let jsxMapper = let component = (nolabel, componentNameExpr) and props = ( nolabel, - Binding.ReactDOM.domProps ~applyLoc:parentExpLoc ~loc:callerLoc props ) + Binding.ReactDOM.domProps ~applyLoc:parentExpLoc ~loc:callerLoc + ~elementName:id props ) in let loc = parentExpLoc in let gloc = { loc with loc_ghost = true } in @@ -645,7 +799,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, @@ -1361,7 +1516,18 @@ let jsxMapper = [@@raises Invalid_argument] method! structure ctxt stru = - super#structure ctxt (reactComponentTransform ~ctxt self stru) + let parentExternals = !externalDeclarations in + externalDeclarations := []; + + let processedStru = + super#structure ctxt (reactComponentTransform ~ctxt self stru) + in + + let allExternals = List.rev !externalDeclarations in + + externalDeclarations := allExternals @ parentExternals; + + allExternals @ processedStru [@@raises Invalid_argument] method! expression ctxt expr = diff --git a/src/ReactDOM.re b/src/ReactDOM.re index 086be0b45..648c11024 100644 --- a/src/ReactDOM.re +++ b/src/ReactDOM.re @@ -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=?, @@ -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] diff --git a/src/ReactDOM.rei b/src/ReactDOM.rei index 75de82b44..b6ffb3e90 100644 --- a/src/ReactDOM.rei +++ b/src/ReactDOM.rei @@ -490,42 +490,47 @@ module Experimental: { type preloadOptions; [@mel.obj] + /* Its possible values are audio, document, embed, fetch, font, image, object, script, style, track, video, worker. */ external preloadOptions: - /* Its possible values are audio, document, embed, fetch, font, image, object, script, style, track, video, worker. */ ( ~_as: [ - | `audio - | `document - | `embed - | `fetch - | `font - | `image - | [@mel.as "object"] `object_ - | `script - | `style - | `track - | `video - | `worker - ], + | `audio + | `document + | `embed + | `fetch + | `font + | `image + | [@mel.as "object"] `object_ + | `script + | `style + | `track + | `video + | `worker + ], /* Suggests a relative priority for fetching the resource. The possible values are auto (the default), high, and low. */ - ~fetchPriority: [ | `auto | `high | `low]=?, + ~fetchPriority: + [ + | `auto + | `high + | `low + ] + =?, /* The Referrer header to send when fetching. Its possible values are no-referrer-when-downgrade (the default), no-referrer, origin, origin-when-cross-origin, and unsafe-url. */ - ~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 - ] - =?, + ~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 + ] + =?, /* For use only with as: "image". Specifies the source set of the image. https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images @@ -563,20 +568,38 @@ module Experimental: { type preinitOptions = { /* possible values: "script" or "style" */ [@mel.as "as"] - _as: [ | `script | `style], + _as: [ + | `script + | `style + ], /* Suggests a relative priority for fetching the resource. The possible values are auto (the default), high, and low. */ [@mel.optional] - fetchPriority: option([ | `auto | `high | `low]), + fetchPriority: + option( + [ + | `auto + | `high + | `low + ], + ), /* Required with Stylesheets (`style). Says where to insert the stylesheet relative to others. Stylesheets with higher precedence can override those with lower precedence. The possible values are reset, low, medium, high. */ [@mel.optional] - precedence: option([ | `reset | `low | `medium | `high]), + precedence: + option( + [ + | `reset + | `low + | `medium + | `high + ], + ), /* a required string. It must be "anonymous", "use-credentials", and "". https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin diff --git a/test/DataAttributes_Compilation__test.re b/test/DataAttributes_Compilation__test.re new file mode 100644 index 000000000..60aa5122d --- /dev/null +++ b/test/DataAttributes_Compilation__test.re @@ -0,0 +1,382 @@ +open Jest; +open Expect; + +[@mel.send] +external getAttribute: (Dom.element, string) => option(string) = + "getAttribute"; + +let getByTestId = (testId, container) => { + ReactTestingLibrary.getByTestId(~matcher=`Str(testId), container); +}; + +type requestStatus = + | Loading + | Success(string) + | Error(string); + +describe("Data Attributes - PPX Compilation Tests", () => { + describe("Basic data attribute compilation", () => { + test( + "should compile data attributes on various element types with proper transformations", + () => { + let container = + ReactTestingLibrary.render( +
+
+ + + +
+ +
+

+ {React.string("Complex structure demo")} +

+
+ , + ); + + // Test various elements in complex structure + let mainElement = getByTestId("complex-structure", container); + let headerElement = getByTestId("app-header", container); + let formElement = getByTestId("test-form", container); + let inputElement = getByTestId("test-input", container); + let buttonElement = getByTestId("submit-button", container); + let footerElement = getByTestId("app-footer", container); + + expect(mainElement->getAttribute("data-component")) + ->toEqual(Some("demo-app")); + expect(headerElement->getAttribute("data-role")) + ->toEqual(Some("app-header")); + expect(formElement->getAttribute("data-validation")) + ->toEqual(Some("required")); + expect(inputElement->getAttribute("data-field")) + ->toEqual(Some("user-data")); + expect(buttonElement->getAttribute("data-action")) + ->toEqual(Some("form-submit")); + expect(footerElement->getAttribute("data-role")) + ->toEqual(Some("app-footer")); + }); + }); + + describe("Data attribute deduplication and reuse", () => { + test( + "should handle same data attributes used multiple times without conflicts", + () => { + let sharedCategory = "test-category"; + + let container = + ReactTestingLibrary.render( +
+
+ {React.string("Item 1")} +
+
+ {React.string("Item 2")} +
+
+ {React.string("Item 3")} +
+ + {React.string("Different element type")} + +
, + ); + + let parentElement = getByTestId("deduplication-test", container); + let item1 = getByTestId("item-1", container); + let item2 = getByTestId("item-2", container); + let item3 = getByTestId("item-3", container); + let spanElement = getByTestId("different-element", container); + + // All elements should have the same category attribute + expect(parentElement->getAttribute("data-category")) + ->toEqual(Some("test-category")); + expect(item1->getAttribute("data-category")) + ->toEqual(Some("test-category")); + expect(item2->getAttribute("data-category")) + ->toEqual(Some("test-category")); + expect(item3->getAttribute("data-category")) + ->toEqual(Some("test-category")); + expect(spanElement->getAttribute("data-category")) + ->toEqual(Some("test-category")); + + // Unique attributes should work correctly + expect(item1->getAttribute("data-index"))->toEqual(Some("1")); + expect(item2->getAttribute("data-index"))->toEqual(Some("2")); + expect(item3->getAttribute("data-index"))->toEqual(Some("3")); + }) + }); +}); diff --git a/test/ReactDOM__test.re b/test/ReactDOM__test.re index 7d82ad6eb..91cb5876d 100644 --- a/test/ReactDOM__test.re +++ b/test/ReactDOM__test.re @@ -1,5 +1,5 @@ open Jest; -open Jest.Expect; +open Expect; module Stream = { type writable = Js.t({.}); @@ -13,6 +13,116 @@ module Stream = { }; describe("ReactDOM", () => { + describe("data attributes JSX support", () => { + test("jsx should render data-* attributes from JSX", () => { + let element =
; + let html = ReactDOMServer.renderToString(element); + + expect(html)->toContain("data-testid=\"my-test\""); + }); + + test("jsx should render data-* attributes with different values", () => { + let element =
; + let html = ReactDOMServer.renderToString(element); + + expect(html)->toContain("data-testid=\"another-test\""); + }); + + test("jsx should render multiple data-* attributes on same element", () => { + let element =
; + let html = ReactDOMServer.renderToString(element); + + expect(html)->toContain("data-testid=\"multi-test\""); + expect(html)->toContain("data-custom=\"value\""); + }); + + test("jsx should handle data attributes with underscores", () => { + let element =
; + let html = ReactDOMServer.renderToString(element); + + expect(html)->toContain("data-test-id=\"underscore-test\""); + }); + + test("jsx should render data attributes on different element types", () => { + let spanElement = ; + let buttonElement =