Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Explore JSX preserve mode (i.e. output jsx syntax in the generated code) #539

Closed
4 tasks
cristianoc opened this issue Jun 13, 2022 · 15 comments
Closed
4 tasks
Labels

Comments

@cristianoc
Copy link
Contributor

cristianoc commented Jun 13, 2022

Rough steps

  • Add configuration to turn off JSX ppx entirely. Separately, figure out where best to expose this configuration. Likely depends on how JSX V4 evolves (e.g. how you configure V3 vs V4 for gradual upgrade).
  • Explore whether the result (i.e. what the JSX ppx uses as input) contains already enough information for preserve mode.
  • Find a way to communicate the information to the compiler's code emitter.
  • Change the compiler's back-end to emit JSX syntax containing compiler-generated code. This should not be very different than a special way to print function application.
@cristianoc cristianoc mentioned this issue Jun 13, 2022
9 tasks
@mununki
Copy link
Member

mununki commented Jun 16, 2022

I'd like to contribute to this feature. Can you share your initial idea to make this done? IMHO, these should be done for this feature.

  • Add compiler flag or configuration in bsconfig.json or rescript.json to preserve JSX
  • Turn the current react ppx off totally. It shouldn't affect for type checker to make the typedtree.
  • Emit JSX with normal js codes from the js representation

@mununki
Copy link
Member

mununki commented Jun 16, 2022

One thing that came to mind is emitting the js codes separately to JSX would be tricky part. Generally, js expressions are written inside JSX.

@mununki
Copy link
Member

mununki commented Jun 19, 2022

Turning off the jsx ppx entirely doesn't seem feasible for the lower case. For example, the parser will generate the lower case jsx into below.

[@JSX] div(~props1=a, ())

The type checker would not know what div is without any binding. Therefore I think the ppx is needed anyway, but it is not only for React. Maybe we need more abstracted modules than rescript-react, react jsx ppx, such as jsx and jsx ppx.

Jsx.createElement("div", { props1: a })

So that it can be re-transformed to the original JSX by js emitter.

@cristianoc
Copy link
Contributor Author

Turning off the jsx ppx entirely doesn't seem feasible for the lower case. For example, the parser will generate the lower case jsx into below.

[@JSX] div(~props1=a, ())

The type checker would not know what div is without any binding. Therefore I think the ppx is needed anyway, but it is not only for React. Maybe we need more abstracted modules than rescript-react, react jsx ppx, such as jsx and jsx ppx.

Jsx.createElement("div", { props1: a })

So that it can be re-transformed to the original JSX by js emitter.

The compiler is pretty smart in generating code. It's totally possible to have a function that just checks the type without affecting the generated code.

@mununki
Copy link
Member

mununki commented Jul 16, 2022

In my opinion, the way to communicate information to the compiler emitter is by using an attribute. But the JSX PPX removes @JSX attribute after transformation. I guess it has to remove the @JSX to prevent the mapper from transforming again in recursively. Therefore, maybe we need another attribute to mark, so let the code emitter identify the JSX.

@cristianoc
Copy link
Contributor Author

I don't think the mapper visits the same node more than once, so it should be no problem.

@mununki
Copy link
Member

mununki commented Jul 17, 2022

If so, there is no reason to remove @JSX attribute from the AST after transformation either. It can be used by compiler emitter also. I'm going to fix the ppx to remain the @JSX.

@mununki
Copy link
Member

mununki commented Jul 17, 2022

Oops, I guess the mapper touches more than once, it throws an error.

Fatal error: exception Invalid_argument("JSX: the JSX attribute should be attached to a `YourModuleName.createElement` or `YourModuleName.make` call. We saw `jsx` instead")

I think I leave it as they are for V4 task for now, then revisit how it communicates to the compiler emitter later.

@cristianoc
Copy link
Contributor Author

What happens if you turn he check off? Does it generated different code than before?

@mununki
Copy link
Member

mununki commented Jul 17, 2022

After turnning off, it prints as below. I think it is related to the printer.

// expected
let make = ({msg}) => ReactDOMRe.createDOMElementVariadic("div", [{msg->React.string}])

// generated
let make = ({msg}) => <ReactDOMRe.createDOMElementVariadic>  </ReactDOMRe.createDOMElementVariadic>

@mununki
Copy link
Member

mununki commented Jul 17, 2022

It seems quite ready to support printing the preserved jsx, I guess.

@cristianoc
Copy link
Contributor Author

Thanks, I guess there's no rush, but it's nice to know that one can preserve that.
Would you put that in an issue to remember to do it later?

@mununki
Copy link
Member

mununki commented Jul 17, 2022

I didn't jump into the preserve jsx feature deeply yet, first thing is first, I still focusing on the JSX PPX V4.
Anyway, I'm getting close to the picture of how to make the preserve feature. It shouldn't be very different as you said.

This should not be very different than a special way to print function application.

I'll leave the issue about it. 👍

@eschaefer
Copy link

@mununki Just registering my interest here 😃 . Would absolutely love this to unlock support for Qwik + Solid (and probably many others) since I love prototyping with Rescript.

@stale
Copy link

stale bot commented May 28, 2023

The rescript-lang/syntax repo is obsolete and will be archived soon. If this issue is still relevant, please reopen in the compiler repo (https://github.com/rescript-lang/rescript-compiler) or comment here to ask for it to be moved. Thank you for your contributions.

@stale stale bot added the stale label May 28, 2023
@stale stale bot closed this as completed Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants