Remove ReactDOM.props

[davesnx]

PR removing ReactDOM.props

[jchavarri]

Sorry, I am not sure I understand the changes, is the removal of props related to the fix for memoCustomCompareProps? Could you share more info about the latter?

[jchavarri]

Were you aware of this? Seems the deletion of props will lead to unsound types?

[davesnx]

Unsound types?

The difference is props.ref is Js.nullable(Dom.element) => unit which is the old api from react where you pass a callback to capture the domNode.

It just not used neither the ppx, neither on our monorepo neither github search: https://github.com/search?q=ReactDOM.props+language%3Aocaml&type=code

[jchavarri]

I don't know. I spent some time trying to understand why this was added but I couldn't find much, besides that it was added when hooks were introduced in #351.

One thing I noticed is that ReactDOM.createDOMElementVariadic is not used anywhere. Maybe it could go as part of this PR.

[jchavarri]

hey @rickyvetter 👋 this is a long shot, as it's been years already, but maybe you remember why two types props and domProps were needed? Just out of pure curiosity :)

[rickyvetter]

Hey folks! Oof this was a while ago. I'm pretty sure this is a combination of:

1. We wanted to support useRef refs only in function components
2. We had to support callback refs in class components
3. We absolutely refused to introduce a runtime
4. I wasn't as good at OCaml as I am now and maybe could've found a better way with those same three constraints today, but at the time this seemed like the obvious/only solution.

[davesnx]

2 is gone now and we aren't strong about 0 runtime neither, I obviously prefer zero runtime when possible but without being super strict.

WDYT @jchavarri ?

[jchavarri]

Thanks Ricky!

I wasn't as good at OCaml as I am now

🤘

we aren't strong about 0 runtime neither, I obviously prefer zero runtime when possible but without being super strict

@davesnx To clarify, there is no runtime addition involved in this PR, right?

WDYT @jchavarri ?

This is some code that breaks after this change:

  let foo = ref(None);
  let setSectionRef = theRef => {
    foo := Js.Nullable.toOption(theRef);
  };
  [@react.component]
  let make = () => {
    ReactDOM.createElement(
      "div",
      ~props=ReactDOM.props(~ref=setSectionRef, ()),
      [||],
    );
  };

However

• I don't expect many people to be calling createElement manually
• and after the jsx additions, we only use this function in the ppx to create fragments, where one can't pass refs (because it's just <>).

So I guess it's safe to do the change? 👍 I would still think it'd be great to document it in the release notes.

[davesnx]

My bad, I was working offline and made the change on the same branch. Changes weren't related. Split the work of memo here: #766

[davesnx]

One thing I noticed is that ReactDOM.createDOMElementVariadic is not used anywhere. Maybe it could go as part of this PR.

To remove the APIs I'm working in #768 and later remove the unused APIs. Does it sound good?

All of this should go before 0.12