Skip to content

Commit b95c627

Browse files
committed
Propagate platform/browser_only attrs to generated include struct
When [@react.component] generates an include struct with makeProps and wrapper make, propagate [@platform ...] and [@browser_only] attributes from the original binding to the include. This allows browser_ppx to drop the entire include on non-matching platforms, fixing the 'Unbound value make' error that occurred when browser_ppx only dropped the internal make binding but left the wrapper.
1 parent 68070f3 commit b95c627

31 files changed

Lines changed: 1920 additions & 227 deletions
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
# Fix OCaml syntax JSX expansion generating incorrect `makeProps` calls
2+
3+
## Problem
4+
5+
In OCaml/mlx syntax, the PPX generates an extra `()` before labeled arguments in `makeProps` calls:
6+
7+
```ocaml
8+
(* BUG: *) Note_header.makeProps () ~title ~preview ~updated_at ()
9+
(* CORRECT: *) Note_header.makeProps ~title ~preview ~updated_at ()
10+
```
11+
12+
Root cause: `strip_final_unit_arg` (line 85-88) only strips `(Nolabel, ())` from the END of the arg list. mlx places `()` at the BEGINNING.
13+
14+
## Changes
15+
16+
### 1. Fix `strip_final_unit_arg``strip_unit_args`
17+
18+
**File**: `packages/server-reason-react-ppx/server_reason_react_ppx.ml`
19+
20+
**Lines 85-88** — Change from:
21+
```ocaml
22+
let strip_final_unit_arg args =
23+
match List.rev args with
24+
| (Nolabel, { pexp_desc = Pexp_construct ({ txt = Lident "()"; _ }, None); _ }) :: rest -> List.rev rest
25+
| _ -> args
26+
```
27+
28+
To:
29+
```ocaml
30+
let strip_unit_args args =
31+
List.filter args ~f:(fun (label, expr) ->
32+
match (label, expr.pexp_desc) with
33+
| Nolabel, Pexp_construct ({ txt = Lident "()"; _ }, None) -> false
34+
| _ -> true)
35+
```
36+
37+
**Line 108** — Update reference from `strip_final_unit_arg` to `strip_unit_args`:
38+
```ocaml
39+
let non_key_args = strip_unit_args non_key_args in
40+
```
41+
42+
### 2. Add OCaml-syntax cram test for uppercase JSX calls
43+
44+
Create `packages/server-reason-react-ppx/cram/upper-calls-ocaml.t/` with:
45+
46+
**`input.ml`** — OCaml-syntax JSX with unit before labeled args (mlx style):
47+
```ocaml
48+
(* Simulates mlx desugaring: <Upper /> *)
49+
let upper = (Upper.createElement () [@JSX])
50+
51+
(* Simulates mlx desugaring: <Upper count /> *)
52+
let upper_prop = (Upper.createElement () ~count [@JSX])
53+
54+
(* Simulates mlx desugaring: <Upper> foo </Upper> *)
55+
let upper_children_single = fun foo -> (Upper.createElement () ~children:[foo] [@JSX])
56+
57+
(* Simulates mlx desugaring: <Foo.Bar a=1 b="1" /> *)
58+
let upper_nested_module = (Foo.Bar.createElement () ~a:1 ~b:"1" [@JSX])
59+
```
60+
61+
**`run.t`** — Cram test verifying correct expansion (no extra `()`):
62+
```
63+
$ ../../standalone.exe --impl input.ml -o output.ml
64+
$ ocamlformat --enable-outside-detected-project --impl output.ml
65+
<expected output showing makeProps ~arg1 ~arg2 () without extra ()>
66+
```
67+
68+
### 3. Run tests
69+
70+
```bash
71+
make build # Rebuild with the fix
72+
make test # All existing tests
73+
make ppx-test # PPX cram tests specifically
74+
```
75+
76+
### 4. Promote snapshots if needed
77+
78+
If existing test output changes (it shouldn't), review diffs:
79+
```bash
80+
make ppx-test-promote
81+
```
82+
83+
## Verification
84+
85+
- All existing Reason-syntax cram tests must still pass unchanged
86+
- New OCaml-syntax test must show `makeProps ~arg ()` (no extra `()`)
87+
- `make build` must succeed
88+
89+
## Issue 2 (Event target types) — No action needed
90+
91+
The `.mli` already defines `target_like` with proper fields (`value`, `checked`, etc.). The bug report's claim of `< >` doesn't match the current code.
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# Plan: Simplify Head & Resource Management in ReactServerDOM.ml
2+
3+
## Goal
4+
Make head management and resource management easier to reason about, without changing behavior.
5+
6+
## Steps
7+
8+
### Step 1: Extract `create_initial_resources`, `create_user_scripts` from render_html (lines 889-987)
9+
10+
Extract three helpers:
11+
- `create_preload_link href` - single preload `<link>` node (deduplicates the two identical `List.map` blocks)
12+
- `create_initial_resources ~bootstrap_scripts ~bootstrap_modules` - preload links for bootstrap resources
13+
- `create_user_scripts ~root_data_payload ?bootstrapScriptContent ?bootstrapScripts ?bootstrapModules ()` - the `user_scripts` list
14+
15+
Then `render_html` calls these instead of building everything inline.
16+
17+
### Step 2: Rename Fiber fields for clarity
18+
19+
| Current | New | Rationale |
20+
|---------|-----|-----------|
21+
| `visited_first_lower_case` | `root_tag` | Communicates purpose: tracking root HTML tag |
22+
| `hoisted_head` | `head_element` | Stores the `<head>` element |
23+
| `hoisted_head_childrens` | `extra_head_children` | Elements promoted into `<head>` (fixes grammar) |
24+
| `html_tag_attributes` | `html_attributes` | Simpler |
25+
26+
Also rename the Fiber setter/getter functions to match.
27+
28+
### Step 3: Extract `reconstruct_document` from render_html (lines 989-1011)
29+
30+
Move the post-render HTML document assembly into:
31+
```ocaml
32+
let reconstruct_document ~fiber ~root_html ~user_scripts ~skip_root = ...
33+
```
34+
35+
This is the single place to understand (or modify) how the final document is assembled, including any future head reordering (issue #303).
36+
37+
### Step 4: Introduce `element_role` classification type
38+
39+
Replace the nested if/match cascade in `render_lower_case_element` with an explicit classification:
40+
41+
```ocaml
42+
type element_role =
43+
| Html_root (* <html> at document root *)
44+
| Head_section (* <head> element *)
45+
| Body_section (* <body> element *)
46+
| Hoistable_resource (* async <script>, <link rel=stylesheet precedence=...> *)
47+
| Hoistable_meta (* <title>, <meta>, <link> outside head *)
48+
| Regular (* everything else *)
49+
50+
let classify_element ~fiber ~tag ~attributes = ...
51+
```
52+
53+
Then `render_lower_case_element` becomes a clean match on this type.
54+
55+
### Verification
56+
Run `make test` after each step. No behavior changes expected.

AGENTS.md

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
# AGENTS.md
2+
3+
## Project Overview
4+
5+
**server-reason-react** is a native OCaml/Reason implementation of React's server-side rendering (SSR) and React Server Components (RSC). It enables writing React components in Reason that compile to both native (server) and JavaScript (client via Melange).
6+
7+
Used in production at ahrefs.com, app.ahrefs.com, and wordcount.com.
8+
9+
## Language & Build System
10+
11+
- **Primary languages**: Reason (`.re`/`.rei`) and OCaml (`.ml`/`.mli`)
12+
- **Build system**: [Dune](https://dune.build/) (version 3.9+)
13+
- **Package manager**: opam (OCaml) + npm (JS tooling)
14+
- **Compiler**: OCaml 5.4.0 (also supports 4.14.1)
15+
- **JS compilation**: [Melange](https://melange.re/) (Reason/OCaml to JavaScript)
16+
- **Formatter**: ocamlformat 0.28.1 (config in `.ocamlformat`, profile=default, margin=120)
17+
18+
## Repository Structure
19+
20+
```
21+
packages/ # Monorepo — all library packages
22+
react/ # Core React module (server-side impl)
23+
reactDom/ # ReactDOM: renderToString, renderToStream, RSC
24+
server-reason-react-ppx/ # JSX PPX transformer for native
25+
browser-ppx/ # PPX to strip browser-only code on server
26+
melange.ppx/ # Melange compatibility PPX for native
27+
Belt/ # Belt standard library (native impl)
28+
Js/ # Js module (native impl, Melange compat)
29+
Dom/ # DOM type stubs
30+
webapi/ # Web API stubs for native compilation
31+
url/ # URL module (dual native/js implementations)
32+
promise/ # Promise module (dual native/js)
33+
html/ # HTML generation utilities
34+
fetch/ # Fetch API stub
35+
runtime/ # Runtime support module
36+
expand-styles-attribute/ # Style attribute expansion PPX
37+
esbuild-plugin/ # Esbuild plugin for client component extraction
38+
react-server-dom-esbuild/ # React Server DOM esbuild integration
39+
demo/ # Demo app (Dream server + client hydration)
40+
benchmark/ # Performance benchmarks
41+
documentation/ # odoc documentation source (.mld files)
42+
arch/ # Architecture reference (browser/server JS)
43+
```
44+
45+
## Essential Commands
46+
47+
```bash
48+
make build # Build the project (dev profile)
49+
make build-prod # Build for production
50+
make test # Run all unit tests (dune build @runtest)
51+
make test-promote # Update test snapshots
52+
make format # Format code with ocamlformat
53+
make format-check # Check formatting (used in CI)
54+
make ppx-test # Run PPX-specific tests
55+
make ppx-test-promote # Promote PPX test snapshots
56+
make demo-serve # Build and serve the demo
57+
make bench # Run benchmarks
58+
make init # Full local setup (switch + deps + npm)
59+
```
60+
61+
## Testing
62+
63+
Three testing strategies are used:
64+
65+
1. **Alcotest unit tests** — Located in `test/` directories within each package. Run with `make test`.
66+
2. **Cram tests** (snapshot/golden tests) — Located as `.t` files in `cram/` or `tests/` directories. These test PPX transformations by comparing actual output against expected snapshots. Promote changes with `make test-promote`.
67+
3. **Inline expect tests** — Some packages use inline tests within source files.
68+
69+
Tests should stay explicit and free of test-only logic. Prefer concrete inputs and direct assertions over conditionals, loops, or helpers that hide the behavior being verified.
70+
71+
When modifying PPX code, always run `make ppx-test` and review snapshot diffs carefully before promoting.
72+
73+
## Key Concepts
74+
75+
### Universal Code
76+
Code that compiles for both native (server) and JavaScript (client). The project uses `copy_files` in dune to share `.re` files between Melange and native library targets. See `documentation/universal-code.mld`.
77+
78+
### PPX Transformations
79+
Three PPXes are central to the project:
80+
- **`server-reason-react-ppx`** — Transforms JSX syntax to native React calls
81+
- **`browser-ppx`** — Strips browser-only code on server (`let%browser_only`, `switch%platform`, `@platform`)
82+
- **`melange.ppx`** — Makes Melange attributes (`mel.*`, `##`, `[%re]`, etc.) work in native
83+
84+
### Dual Implementations
85+
Some modules have separate native and JS implementations sharing the same interface:
86+
- `packages/url/native/` vs `packages/url/js/`
87+
- `packages/promise/native/` vs `packages/promise/js/`
88+
89+
### React Server Components (Experimental)
90+
RSC support includes `ReactServerDOM`, an esbuild plugin for client component extraction, and Dream middleware. This is still work in progress.
91+
92+
## Coding Conventions
93+
94+
- Reason (`.re`) is preferred for React components and new application code
95+
- OCaml (`.ml`) is used for library internals, PPX implementations, and Belt/Js modules
96+
- Interface files (`.mli`/`.rei`) are used to define public APIs
97+
- Each package has its own `dune` file defining libraries and test targets
98+
- Follow existing formatting — run `make format` before committing
99+
- PPX transformations use `ppxlib` and follow its visitor pattern
100+
101+
## OCaml
102+
103+
- Do not use `Obj.magic` or `%identity` unless it is absolutely necessary.
104+
- Avoid both at all costs; prefer type-safe alternatives even if they require a slightly larger refactor.
105+
- If either appears to be the only viable option, stop and ask the user before introducing it.
106+
107+
## Do NOT
108+
109+
- Modify `_build/`, `_opam/`, or `node_modules/` directories
110+
- Change `.ocamlformat` settings without discussion
111+
- Skip running tests after modifying PPX code — PPX changes can silently break downstream code
112+
- Introduce new opam dependencies without updating `dune-project`
113+
- Assume browser APIs are available on the server — use `let%browser_only` or `switch%platform`
114+
115+
## CI
116+
117+
GitHub Actions (`.github/workflows/ci.yml`) runs on push to `main` and PRs:
118+
- Matrix: `{macos, ubuntu}` x `{OCaml 4.14.1, OCaml 5.4.0}`
119+
- Steps: build, format check (OCaml 5+ only), tests, docs generation, benchmarks
120+
121+
## Useful Links
122+
123+
- [Documentation](https://ml-in-barcelona.github.io/server-reason-react/server-reason-react/index.html)
124+
- [Reason syntax docs](https://reasonml.github.io/docs/en/what-and-why)
125+
- [Melange docs](https://melange.re/)
126+
- [Dune docs](https://dune.readthedocs.io/)
127+
- [ppxlib docs](https://ocaml.org/p/ppxlib/latest/doc/index.html)

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ demo-serve: demo-build ## Serve the demo executable
103103

104104
.PHONY: demo-serve-watch
105105
demo-serve-watch: ## Run demo executable on watch mode (listening to built_at.txt changes)
106-
@watchexec --no-vcs-ignore -w demo/.running/built_at.txt -r -c -- "_build/default/demo/server/server.exe"
106+
@watchexec --no-vcs-ignore -w demo/.running/built_at.txt -r -c -- "DEMO_ENV=development _build/default/demo/server/server.exe"
107107

108108
.PHONY: subst
109109
subst: ## Run dune substitute

demo/client/DummyRouterRSC.re

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,14 @@ let callServer = (path: string, args) => {
2727
});
2828
ReactServerDOMEsbuild.encodeReply(args)
2929
|> Js.Promise.then_(body => {
30-
let body = Fetch.BodyInit.make(body);
3130
Fetch.fetchWithInit(
3231
"/",
3332
Fetch.RequestInit.make(~method_=Fetch.Post, ~headers, ~body, ()),
3433
)
3534
|> Js.Promise.then_(result => {
3635
let body = Fetch.Response.body(result);
3736
ReactServerDOMEsbuild.createFromReadableStream(body);
38-
});
37+
})
3938
});
4039
};
4140

demo/client/NestedRouterRSC.re

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,14 @@ let callServer = (path: string, args) => {
1313
});
1414
ReactServerDOMEsbuild.encodeReply(args)
1515
|> Js.Promise.then_(body => {
16-
let body = Fetch.BodyInit.make(body);
1716
Fetch.fetchWithInit(
1817
"/",
1918
Fetch.RequestInit.make(~method_=Fetch.Post, ~headers, ~body, ()),
2019
)
2120
|> Js.Promise.then_(result => {
2221
let body = Fetch.Response.body(result);
2322
ReactServerDOMEsbuild.createFromReadableStream(body);
24-
});
23+
})
2524
});
2625
};
2726

demo/client/SinglePageRSC.re

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@ let callServer = (path: string, args) => {
66
});
77
ReactServerDOMEsbuild.encodeReply(args)
88
|> Js.Promise.then_(body => {
9-
let body = Fetch.BodyInit.make(body);
109
Fetch.fetchWithInit(
1110
"/",
1211
Fetch.RequestInit.make(~method_=Fetch.Post, ~headers, ~body, ()),
1312
)
1413
|> Js.Promise.then_(result => {
1514
let body = Fetch.Response.body(result);
1615
ReactServerDOMEsbuild.createFromReadableStream(body);
17-
});
16+
})
1817
});
1918
};
2019

demo/dream-nested-router/js/HistoryState.re

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,30 @@
11
module DOM = Webapi.Dom;
2-
module History = DOM.History;
32

4-
/**
5-
* Melange webapi don't set state type, so we use Obj.magic to cast it to the correct type while the PR is not merged.
6-
* https://github.com/melange-community/melange-webapi/blob/80c6ededd06cc66b75445d1ed5c855e050b156a0/src/Webapi/Dom/Webapi__Dom__History.re#L2
7-
* PR: https://github.com/melange-community/melange-webapi/pull/29
8-
*/
9-
[@platform js]
10-
type t = History.state;
3+
/* Custom History bindings with polymorphic state, replacing the upstream opaque type from melange-webapi. Also waiting for the PR to be merged: https://github.com/melange-community/melange-webapi/pull/29 */
4+
module History = {
5+
type t = Dom.history;
6+
7+
[@mel.get] external state: t => 'a = "state";
8+
9+
[@mel.send]
10+
external pushState: ([@mel.this] t, 'a, string, string) => unit =
11+
"pushState";
12+
13+
[@mel.send]
14+
external replaceState: ([@mel.this] t, 'a, string, string) => unit =
15+
"replaceState";
16+
};
1117

1218
let fromEvent = event =>
1319
DOM.Event.target(event)
1420
->DOM.EventTarget.unsafeAsWindow
1521
->DOM.Window.history
1622
->History.state;
1723

18-
let toJs: History.state => Js.t({..}) = state => state |> Obj.magic;
19-
let fromJs: Js.t({..}) => History.state = state => state |> Obj.magic;
20-
2124
[@platform js]
2225
let push = (state, path) => {
23-
History.pushState(state, "", path, DOM.history);
24-
let _ =
26+
History.pushState(DOM.history, state, "", path);
27+
let (_: bool) =
2528
DOM.EventTarget.dispatchEvent(
2629
DOM.Event.make("popstate"),
2730
DOM.Window.asEventTarget(DOM.window),
@@ -31,8 +34,8 @@ let push = (state, path) => {
3134

3235
[@platform js]
3336
let replace = (state, path) => {
34-
History.replaceState(state, "", path, DOM.history);
35-
let _ =
37+
History.replaceState(DOM.history, state, "", path);
38+
let (_: bool) =
3639
DOM.EventTarget.dispatchEvent(
3740
DOM.Event.make("popstate"),
3841
DOM.Window.asEventTarget(DOM.window),

0 commit comments

Comments
 (0)