Skip to content

WIP - Try and transform jsx through compilation #7385

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 1 commit into
base: master
Choose a base branch
from

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Apr 9, 2025

Super rough experiment about type-checking the new Jsx untyped nodes.
Current attempt is to compile let x = <div className="yow"></div> and print jsx in js_dump.

Current result:

image

Copy link
Collaborator Author

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Hello @cristianoc, curious to hear your thoughts on this!
//cc @zth

false )
| _ -> None)
in
let record = Ast_helper.Exp.record fields None in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This idea here is to create a record of JsxDOM.domProps and type-check that.
It is of course laser focused to the <div> example.

(children could later be checked separately, as I believe we need to check if those Jsx.element)

let path = Path.Pdot (Path.Pident (Ident.create "Jsx"), "element", 0) in
Ctype.newconstr path []
in
let _typed_record = type_expect_ env record jsx_dom_type_expected in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

jsx_dom_type_expected isn't quite right here. Could we maybe try and find it in the env?

let _typed_record = type_expect_ env record jsx_dom_type_expected in
{
exp_desc =
Texp_jsx_container_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.

We transform this node a couple of times later on, in all the phases but overall the important check happened here.

@nojaf nojaf linked an issue Apr 9, 2025 that may be closed by this pull request
@cristianoc
Copy link
Collaborator

After looking at how complexity builds up, perhaps this was not the best suggestion.
If the result of the normal transformation is a form of application, then that application seems to contain almost all the information needed. E.g. ReactDOM.jsx("div", ...).
Perhaps a simpler way is to add some info to the application, so that it's treated differently after type checking.
That seems easy for things like div, not sure about the other cases.
Ideally, we'd want not to complicate type checking, but at the same time keep enough information to emit code in preserve mode.
Perhaps one can add a bit more information to the application ast node, and if useful modify a bit the ppx too if reconstructing the info for preserve mode is more complex than it needs to be.

Thoughts?

@nojaf
Copy link
Collaborator Author

nojaf commented Apr 10, 2025

I agree that this touches on many layers, and the impact of breaking something is considerable.

Marking the transformed application call with additional data could also be effective. I would support adding the original jsx_element node as optional data in Pexp_apply. This information needs to be carried over every time the application gets transformed, as I don't see any way around that.

We could ultimately detect it in Js_dump and reformulate it as JSX. This might be feasible, and I'll experiment with this idea further, see #7387

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.

JSX Preserve Mode
2 participants