Put JSX 4 PPX support functions into a separate module

rescript-lang / rescript-react

Official ReScript bindings for ReactJS

https://rescript-lang.org/docs/react/latest/introduction

MIT License

478 stars 39 forks source link

Put JSX 4 PPX support functions into a separate module #76

Closed cknitt closed 2 years ago

cknitt commented 2 years ago

Currently, the React module has the functions createElementWithKey and createElementVariadicWithKey.

In my understanding, these are helper functions required for the JSX 4 PPX, but are not meant to be used by a user of the React bindings.

I would therefore propose to move them to a separate module, something like ReactPPX4Support. What do you think @mattdamon108?

cristianoc commented 2 years ago

@mattdamon108 there was a bit of back and forth about where to put the source of truth for these things. Since the source of truth for types is now the compiler's Jsx e.g. type element = Jsx.element, I guess it makes sense to also put these functions there, unless there are some issues with this, which were discussed but I have forgotten?

mununki commented 2 years ago

Sorry for missing the notification. First of all, adding Jsx is a first step to embracing other JSX libraries besides the React. Therefore, I added the primitives such as types there. Second, we removed the function such as addKeyProp was the inline issue, as I remember. https://github.com/rescript-lang/rescript-compiler/pull/5677#discussion_r975274485

cknitt commented 2 years ago

Ok, so my understanding is:

the Jsx stuff is meant to be framework-neutral and is part of the compiler repo
the PPX is React-specific and is also part of the compiler repo
the PPX requires some helper functions that are currently part of @rescript/react, not of the compiler repo
addKeyProp needs to be colocated with createElementWithKey and createElementVariadicWithKey

Correct?

So my suggestion would be to move addKeyProp, createElementWithKey and createElementVariadicWithKey out of @rescript/react and into a new "PPX support module" in the compiler repo. Which means that we'd also need to duplicate the corresponding React.createElement bindings there. But that should be fine.

This way the PPX + its support code can evolve independently from @rescript/react, and users of the React bindings are not tempted to invoke some PPX-internal functions.

mununki commented 2 years ago

Yes, correct.

I agreed. If we move the helper functions out of rescript-react, then non-zero cost js output would be cleaned from it. It would make the rescript-react a binding module.

cristianoc commented 2 years ago

Sounds great.

mununki commented 2 years ago

@cknitt Can we close it?

cknitt commented 2 years ago

Yes, done 😄