Skip to content

WIP - Extra app data #7387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Apr 10, 2025

@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.

@@ -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} ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Store the potentially transformed jsx node.

@@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Capture jsx ast

@@ -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} ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh looks like it gets lost in bs_ast_mapper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@@ -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}) ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@nojaf nojaf changed the title Extra app data WIP - Extra app data Apr 10, 2025
@nojaf
Copy link
Collaborator Author

nojaf commented Apr 23, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants