Props types cleanup

[cknitt]

@mattdamon108 I have a question regarding props types:

Why are domProps and props as defined in

rescript-react/src/ReactDOM.res

Line 58 in d5d9168

type domProps = {

and

rescript-react/src/ReactDOM.res

Line 1085 in d5d9168

type props = {

still @deriving(abstract) types and not just

type props = JsxDOM.props
type domProps = JsxDOM.domProps

?

[cknitt]

Also, in the JsxDOM module itself, if JsxDOM.props and JsxDOM.domProps are the same except for the ref type like the comment says, why can't we have just have something like

type _props<'ref> = {
  ref: 'ref,
  // all the other props
}

type domProps = _props<domRef>
type props = _props<domRef => unit>

instead of keeping the definitions in sync manually?

[mununki]

That is very good question! I’m still not solving the puzzle why ReactDOM.props and ReactDOM.domProps were defined seperately.
Other questions are in ongoing task. Actually, current status of JSX v4 is not changing the lowecase such as <div /> against v3. The lowercase are using those types.

I think next step of JSX v4 is changing the internal representation for the lowercase without ReactDOM.domProps() magic.

[cknitt]

@mattdamon108 Ok, so basically the props types are still WIP and you are planning to unify them soon as part of the JSX v4 work?

The reason I was asking is that there are many open issues and PRs about some adding some missing prop, and it would be ideal to be able to do that in a single place.

So if I want to add missing props now, I guess I should do it in the JsxDOM module, and the types in rescript-react will then be changed to point to the JsxDOM types in the course of your JSX v4 work?

[mununki]

@mattdamon108 Ok, so basically the props types are still WIP and you are planning to unify them soon as part of the JSX v4 work?

The reason I was asking is that there are many open issues and PRs about some adding some missing prop, and it would be ideal to be able to do that in a single place.

So if I want to add missing props now, I guess I should do it in the JsxDOM module, and the types in rescript-react will then be changed to point to the JsxDOM types in the course of your JSX v4 work?

I understood what you need.
Unifying domProps and props and putting it to one place JsxDOM, need to be done.
(As I quick look, I think ref in type JsxDOM.props is the old way to plant the ref and discouraged way also.)

@cristianoc Can I start now for v10.1 or next? IMHO, it would be better to change the internal representation for the lowercase at the same time.

[cristianoc]

Starting now sounds good.
It would be good to understand why things are the way they are. Eg there's old code that bypasses this for Eg aria. And it would be good to understand what breaks and if there's anything to be concerned.

A long way to say: why not make a plan and post on the forum for feedback.

[mununki]

A long way to say: why not make a plan and post on the forum for feedback.

Good point. Let me leave thoughts here first, then refine them to post on the forum.

[mununki]

Rough thoughts to ask to the forum:

1. using ReactDOM.props or ReactDOM.domProps in user space code base?
2. <div ref={ref => divRef.current = ref} /> still using? <-- ReactDOM.props

Anything else?

[cristianoc]

Something about the runtime representation. If they move from deriving abstract to something else, is user code affected?

[mununki]

Something about the runtime representation. If they move from deriving abstract to something else, is user code affected?

3. changing ReactDOM.domProps(~id="foo", ariaDetails="bar", ()) -> {id: "foo", ariaDetails: "bar"} what affects user code.

[cknitt]

AFAIK the runtime representation is the same in both cases, just the JS object {id: 'foo', ariaDetails: 'bar'}.

The question is if there is user code using the ReactDOM.domProps creation function or one of the generated getters and setters, but I doubt that is the case. At least I am sure we are not using this in any of our codebases.

[mununki]

https://forum.rescript-lang.org/t/rfc-reactdom-props-type/3790

[mununki]

• compiler Merge type domProps and type props in JsxDOM rescript#5706
• syntax JSX v4 record-based representation for lowercase syntax#665
• rescript-react Deprecate ReactDOM.props & compat to JSX v4 lowercase #63

[cknitt]

So with the changes in #63 any DOM props updates will be performed in a single place in the compiler repo's JsxDOM.domProps only, and the deprecated ReactDOM.props just stays the way it is now (until it is eventually removed). Agreed?

[mununki]

Agreed!

[cknitt]

Why is there actually

module Props = {
  type domProps = JsxDOM.domProps
  ...
  type props = ...
}

include Props

instead of just defining domProps and props directly in the outer scope?

(The generated accessors for props will pollute the outer scope in any case.)

[mununki]

I have no answer about it, I'm wondering why too. It is included also in ReactDOMRe too.
I guess there should be a reason in back then, no idea.

[cknitt]

Maybe we should just deprecate everything in the Props submodule:

type domProps = JsxDOM.domProps

/** DEPRECATED */
module Props = {
  @deprecated("Please use type ReactDOM.domProps")
  type domProps = JsxDOM.domProps

  @deriving(abstract)
  @deprecated("Please use type ReactDOM.domProps")
  type props = {
    ...
  }
}

[mununki]

In my humble guess, maybe there was a need to contain two kind of props there?

[mununki]

Maybe we should just deprecate everything in the Props submodule:

type domProps = JsxDOM.domProps

/** DEPRECATED */
module Props = {
  @deprecated("Please use type ReactDOM.domProps")
  type domProps = JsxDOM.domProps

  @deriving(abstract)
  @deprecated("Please use type ReactDOM.domProps")
  type props = {
    ...
  }
}

If so, ReactDOMRe can not include Props.

[mununki]

JSX v3 is still using ReactDOMRe.domProps

[mununki]

// ReactDOMRe.res
type domProps = JsxDOM.props

Maybe it is needed too.

[cknitt]

Do you mean

@deprecated("Please use type ReactDOM.domProps")
type props = JsxDOM.domProps

in the root scope?

I will create a PR.

[mununki]

Do you mean

@deprecated("Please use type ReactDOM.domProps")
type props = JsxDOM.domProps

in the root scope?

Actually not.

I guess,

• ReactDOM.res -> Deprecating the module Props, and add type domProps in the root scope
• ReactDOMRe.res -> include ReactDOM.Props -> ReactDOM_V3.Props

[cknitt]

Ah, right, sorry, I misunderstood, overlooked the "Re" in ReactDOMRe". 😉

[cknitt]

I think this is done now, at least as far as the rescript-react repo is concerned.