rescript-lang / syntax

ReScript's syntax as a standalone repo.
MIT License
254 stars 38 forks source link

More generalized props type for ref and type variables #637

Closed mununki closed 2 years ago

mununki commented 2 years ago

This PR has two purposes.

1. The props type with unbounded type variables

Fix the issue https://forum.rescript-lang.org/t/call-for-help-test-the-react-jsx-v4/3713/57?u=moondaddi, which is how to handle type variables for making props type.

type t<'a>

// original
external make: (~ref: t<'a>) => React.element = "..."

// converted to
// before
type props = {ref: t<'a>} // Unbound type parameter 'a
external make: React.componentLike<props, React.element> = "..."

// after
type props<'ref> = {ref: 'ref}
external make: React.componentLike<props<t<'a>>, React.element> = "..."
}

2. More generalized type for ref

The above issue reminds me that the constraint of type ref. The type constraint ReactDOM.Ref.currentDomRef is not flexible in case the JSX ppx is used with other jsx libraries, such as React Native. The type of ref would be different in each library.

Without type annotation for ref, ReactDOM.Ref.currentDomRef as default

// original
@react.component React.forwardRef((~x, ref) => body)

// before
type props<'x> = {
  ref?: ReactDOM.Ref.currentDomRef,
  x: x,
}

({x, _}: props<'x>, ref) => body

// after
type props<'x, 'ref> = {
  x: 'x,
  ref?: 'ref,
}

({x, _}: props<'x, ReactRef.currentDomRef>, ref) => body

With type annotation

// original
@react.component React.forwardRef((~x, ref: Js.Nullable.t<ReactNative.Ref.t>) => body)

// after
type props<'x, 'ref> = {
  x: 'x,
  ref?: 'ref,
}

({x, _}: props<'x, ReactNative.Ref.t>, ref: Js.Nullable.t<ReactNative.Ref.t>) => {
  let _ = ref->Js.Nullable.toOption->Belt.Option.map(ReactNative.Ref.domRef)
}

Js.Nullable.t<'ref> is used for the runtime safety instead of option<'ref>, because the value of ref could be null. 'ref should be used in the props type for calling from the other application sites.

cristianoc commented 2 years ago

Would you rebase past this? https://github.com/rescript-lang/syntax/pull/638 Or re-create in case the changes are too big.

cristianoc commented 2 years ago

@mattdamon108 For 1, what's the idea? It looks like in the original code there was a constraint on type t. Presumably the user intended to have that constraint. Is that completely gone now?

cristianoc commented 2 years ago

@mattdamon108 For 1, what's the idea? It looks like in the original code there was a constraint on type t. Presumably the user intended to have that constraint. Is that completely gone now?

Sorry I did not see it's there. Ignore me. React.componentLike<props<t<'a>>

mununki commented 2 years ago

Would you rebase past this? #638 Or re-create in case the changes are too big.

Done!

mununki commented 2 years ago

Do all the examples compile fine?

It is fine in the example project. I’m going to run more in my company project. Let you know the result.

Perhaps 1 more line of comment on where the removed Js.Nullable.t is coming from (2nd arg of forwardRef). Should Js.nullable be removed in addition of Js.Nullable.t, in case some details of the bindings change in future?

Can I ask what you mean the removed Js.Nullable.t? Is it going to be removed?

cristianoc commented 2 years ago

It is fine in the example project. I’m going to run more in my company project. Let you know the result.

Great. I'll merge assuming there are no issues. If there are, we can revisit.

Can I ask what you mean the removed Js.Nullable.t? Is it going to be removed?

Sorry I meant function stripJsNullable.

mununki commented 2 years ago

Sorry I meant function stripJsNullable.

Ah! It’s little tricky part, though. If the 2nd arg of forwardRef can be sure not a null value, we can use the option type, instead of Js.Nullable.t.

Not a very clear picture how to improve this part now, but we can tackle this part later to improve more cleaner and less magic.