[WIP] - Preserve JSX by adding extra app flag #7387

nojaf · 2025-04-10T12:35:52Z

@cristianoc, this doesn't work yet, but it highlights the idea. Could you point out where the information is not passed? It's a bit overwhelming for me to grasp.

nojaf · 2025-04-10T12:36:17Z

compiler/ml/tast_mapper.ml

@@ -199,12 +199,13 @@ let expr sub x =
    | Texp_function {arg_label; arity; param; case; partial; async} ->
      Texp_function
        {arg_label; arity; param; case = sub.case sub case; partial; async}
-    | Texp_apply {funct = exp; args = list; partial} ->
+    | Texp_apply {funct = exp; args = list; partial; transformed_jsx} ->


Store the potentially transformed jsx node.

nojaf · 2025-04-10T12:36:36Z

compiler/syntax/src/jsx_v4.ml

@@ -1277,7 +1278,7 @@ let mk_react_jsx (config : Jsx_common.jsx_config) mapper loc attrs
        [key_prop; (nolabel, unit_expr ~loc:Location.none)] )
  in
  let args = [(nolabel, elementTag); (nolabel, props_record)] @ key_and_unit in
-  Exp.apply ~loc ~attrs jsx_expr args
+  Exp.apply ~loc ~attrs ~transformed_jsx jsx_expr args


Capture jsx ast

nojaf · 2025-04-10T12:36:57Z

compiler/ml/typecore.ml

@@ -2401,7 +2401,7 @@ and type_expect_ ?type_clash_context ?in_function ?(recarg = Rejected) env sexp
    type_function ?in_function ~arity ~async loc sexp.pexp_attributes env
      ty_expected l
      [Ast_helper.Exp.case spat sbody]
-  | Pexp_apply {funct = sfunct; args = sargs; partial} ->
+  | Pexp_apply {funct = sfunct; args = sargs; partial; transformed_jsx} ->


From here on, pass it along to the next layer.

Looks like it never makes it here. So it must have gotten lost somewhere.
I'd start with a simple test examples and start adding some assert false whenever transformed_jsx is not None, to catch where was the last point where it was present before getting lost.

Oh looks like it gets lost in bs_ast_mapper.

Yeah, it seems to take a different route because <div> is an FFI call.

nojaf · 2025-04-10T12:37:43Z

compiler/core/js_dump.ml

@@ -524,6 +524,10 @@ and expression_desc cxt ~(level : int) f x : cxt =
         when Ext_list.length_equal el i
       ]}
    *)
+  | Call (e, el, {call_transformed_jsx = Some jsx_element}) ->


Hope it ends up here and do something meaningful.
Unfortunately, it didn't work for me. It got lost somewhere.

nojaf · 2025-04-23T11:44:23Z

Got an initial result here, was able to transform:

module ReactDOM = {
    @module("react/jsx-runtime")
    external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}

let _ = <div className="meh"></div>

into

// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';

let JsxRuntime = require("react/jsx-runtime");

let ReactDOM = {};

<div className={"meh"}></div>;

exports.ReactDOM = ReactDOM;

There are still a lot of cases to cover, but it is a start.

nojaf · 2025-04-25T07:16:13Z

Made some more progress, was able to transform my entire project.
This is an example of how the playground example looks like:

// Generated by ReScript, PLEASE EDIT WITH CARE

import * as React from "react";
import * as JsxRuntime from "react/jsx-runtime";

function Playground$CounterMessage(props) {
  let username = props.username;
  let count = props.count;
  let times = count !== 1 ? (
      count !== 2 ? String(count) + " times" : "twice"
    ) : "once";
  let name = username !== undefined && username !== "" ? username : "Anonymous";
  return <div>{"Hello " + name + ", you clicked me " + times}</div>;
}

let CounterMessage = {
  make: Playground$CounterMessage
};

function Playground$App(props) {
  let match = React.useState(() => 0);
  let setCount = match[1];
  let match$1 = React.useState(() => "Anonymous");
  let setUsername = match$1[1];
  let username = match$1[0];
  return <div>{[
    "Username: ",
    <input type={"text"} value={username} onChange={event => {
      event.preventDefault();
      let eventTarget = event.target;
      let username = eventTarget.value;
      setUsername(_prev => username);
    }}/>,
    <button onClick={_evt => setCount(prev => prev + 1 | 0)}>{"Click me"}</button>,
    <button onClick={_evt => setCount(param => 0)}>{"Reset"}</button>,
    <Playground$CounterMessage count={match[0]} username={username}/>
  ]}</div>;
}

let App = {
  make: Playground$App
};

export {
  CounterMessage,
  App,
}
/* react Not a pure module */

pkg-pr-new · 2025-05-03T12:58:08Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7387

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7387

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7387

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7387

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7387

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7387

commit: 90b28f0

This reverts commit 547dfca.

nojaf · 2025-05-03T14:03:40Z

@cknitt you can test this by:

npm i "https://pkg.pr.new/rescript-lang/rescript@aa94f6fdbed6156c46ac4511fa1029a8fbcd553d"

rescript.json:

  "suffix": ".res.jsx",
  "jsx": {
    "version": 4
  },
 "bsc-flags": [
    "-bs-jsx-preserve"
  ]

(suffix not strictly required)

nojaf · 2025-05-03T14:05:20Z

@cristianoc, this is now functioning effectively. Could you please review this approach? If it seems reasonable, we can refine it.

cristianoc

Looks pretty good.
Main question is if there's a way to avoid extending every Lprim with a boolean, and the scope of changes can be restricted somehow.

cristianoc · 2025-05-04T08:07:30Z

compiler/bsc/rescript_compiler_main.ml

@@ -255,6 +255,9 @@ let buckle_script_flags : (string * Bsc_args.spec * string) array =
    ( "-bs-jsx-mode",
      string_call ignore,
      "*internal* Set jsx mode, this is no longer used and is a no-op." );
+    ( "-bs-jsx-preserve",
+      unit_call (fun _ -> Js_config.jsx_preserve := true),


Can just use set instead of unit_call

cristianoc · 2025-05-04T08:09:24Z

analysis/src/CompletionFrontEnd.ml

@@ -294,6 +297,7 @@ let rec exprToContextPathInner ~(inJsxContext : bool) (e : Parsetree.expression)
              funct = {pexp_desc = Pexp_ident id; pexp_loc; pexp_attributes};
              args = [(Nolabel, lhs)];
              partial;
+              transformed_jsx;


perhaps give a name to the inline record passed to Pexp_apply in the pattern match, and here only mention the changed cases (so no mention of partial and transformed_jsx).

cristianoc · 2025-05-04T08:11:12Z

compiler/core/js_dump.ml

@@ -524,6 +524,48 @@ and expression_desc cxt ~(level : int) f x : cxt =
         when Ext_list.length_equal el i
       ]}
    *)
+  | Call


perhaps add comment to explain what this does

cristianoc · 2025-05-04T08:12:39Z

compiler/core/js_dump.ml

+      let fields = ("key", key) :: fields in
+      print_jsx cxt ~level f fnName tag fields
+    | _ ->
+      expression_desc cxt ~level f


Is this code branch ever executed?

If it is, it means we missed something. Could we add some sort of assert in this case?

cristianoc · 2025-05-04T08:18:07Z

compiler/core/lam_compile_external_call.ml

-let translate_ffi (cxt : Lam_compile_context.t) arg_types
-    (ffi : External_ffi_types.external_spec) (args : J.expression list)
-    ~dynamic_import =
+let translate_ffi ?(transformed_jsx = false) (cxt : Lam_compile_context.t)


Do we get transformed_jsx true sometimes when compiling an external call?
Can't remember if the jsx ppx produced external calls.

cristianoc · 2025-05-04T08:19:28Z

compiler/core/lam_compile_primitive.ml

    (E.js_global "import")
    [E.str path]

 let wrap_then import value =
  let arg = Ident.create "m" in
  E.call
-    ~info:{arity = Full; call_info = Call_na}
+    ~info:{arity = Full; call_info = Call_na; call_transformed_jsx = false}


Possible cleanup: define some common info values to avoid all these repetitions.

nojaf · 2025-05-04T10:02:37Z

@cristianoc could you check e30cab3 and 773124f as well. Had to write some new to deal with prop spreading.

nojaf commented Apr 10, 2025

View reviewed changes

nojaf mentioned this pull request Apr 10, 2025

WIP - Try and transform jsx through compilation #7385

Closed

nojaf changed the title ~~Extra app data~~ WIP - Extra app data Apr 10, 2025

nojaf changed the title ~~WIP - Extra app data~~ WIP - Preserve JSX by adding extra app flag Apr 28, 2025

nojaf mentioned this pull request Apr 28, 2025

JSX Preserve Mode #6197

Open

nojaf changed the title ~~WIP - Preserve JSX by adding extra app flag~~ [WIP] - Preserve JSX by adding extra app flag Apr 30, 2025

nojaf added 9 commits May 3, 2025 12:38

Add additional node to Pexp_apply

ffc7666

Try and pass jsx_element from typed_tree to js_call

92e38cb

Follow Lprim

dd56e61

Transform initial simple element

a0c170f

Initial fragment support

a98e3b8

WIP extract, good stuff

1801da1

Print props, catch with key functions

ab32462

Don't pass untyped ast, simple flag is sufficient.

6b075c0

Support fragments

d0bfedc

nojaf force-pushed the extra-app-data branch from a58f8c9 to d0bfedc Compare May 3, 2025 10:38

nojaf added 4 commits May 3, 2025 14:23

Unwrap children

3d2930e

Poor man feature flag

547dfca

Remove duplicated in

09950f1

Older camls

0ab6987

nojaf added 2 commits May 3, 2025 15:28

Revert "Poor man feature flag"

8e4811b

This reverts commit 547dfca.

Add new -bs-jsx-preserve flag

aa94f6f

nojaf requested a review from cristianoc May 3, 2025 14:05

cristianoc reviewed May 4, 2025

View reviewed changes

WIP, deal with prop spreading

e30cab3

Deal with prop spreading

773124f

cristianoc and others added 7 commits May 4, 2025 12:12

Clean up Lprim and move the information to the call primitive.

46d3d50

Add test for spreading children

1a20f38

Refactor duplicate code

8929f37

Support keyed and prop spreading

cecf67d

Rename test file

3fcf1df

Keep key prop before spreading props. Also ensure test can run.

8b7eb45

Add only spread props case

90b28f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - Preserve JSX by adding extra app flag #7387

[WIP] - Preserve JSX by adding extra app flag #7387

nojaf commented Apr 10, 2025

nojaf Apr 10, 2025

nojaf Apr 10, 2025

nojaf Apr 10, 2025

cristianoc Apr 11, 2025

cristianoc Apr 11, 2025

nojaf Apr 11, 2025

nojaf Apr 10, 2025

nojaf commented Apr 23, 2025

nojaf commented Apr 25, 2025

pkg-pr-new bot commented May 3, 2025 •

edited

Loading

nojaf commented May 3, 2025

nojaf commented May 3, 2025

cristianoc left a comment

cristianoc May 4, 2025

cristianoc May 4, 2025

cristianoc May 4, 2025

cristianoc May 4, 2025

nojaf May 4, 2025

cristianoc May 4, 2025

cristianoc May 4, 2025

nojaf commented May 4, 2025

[WIP] - Preserve JSX by adding extra app flag #7387

Are you sure you want to change the base?

[WIP] - Preserve JSX by adding extra app flag #7387

Conversation

nojaf commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Apr 23, 2025

nojaf commented Apr 25, 2025

pkg-pr-new bot commented May 3, 2025 • edited Loading

nojaf commented May 3, 2025

nojaf commented May 3, 2025

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented May 4, 2025

pkg-pr-new bot commented May 3, 2025 •

edited

Loading