Bug: JSX 4 doesn't work with prop having var with the same name as default value

[DZakh]

https://rescript-lang.org/try?version=v11.0.0-beta.4&code=ALBWGcA8GEHsDsBmBLA5gCgN4DcCmAncZBALgAIAWAGjIFtYATXcgIgEMBXAF1lra+QBjFgF8AlACgJ9BhwA2uMgHVY+ANbgyAXjKYJZMgq5kARtrItEsWC31lg+XG0FcAdIN4AHBLnhc7RnRsaoo66AB+bFomYtoAfGQASk4uruBc+MjwGGySIlIy8ooAIrC44H4q6uZ6BoFs5pbWtgYOKW4etN7wvv51uMZ8IeYRUbnxSe1pGVk5eRJAA

[mununki]

Good catch! We can see why by looking at the code that JSX PPX converted.

module DoesntWork = {
  let a = "foo" // 1
  type props<'a> = {a?: 'a}

  let make = ({?a, _}: props<_>) => { // 2
    let a = switch a {
    | Some(a) => a
    | None => a // <- this should refer to the 1, not 2
    }

    React.string(a)
  }
  ...
}

[mununki]

Maybe we can fix this using alias the props name:

module DoesntWork = {
  let a = "foo"
  type props<'a> = {a?: 'a}

  let make = ({a: ?a_, _}: props<_>) => { // alias a as a_
    let a = switch a_ { // refer a_ here
    | Some(a) => a
    | None => a
    }

    React.string(a)
  }
  ...
}

[mununki]

What do you think? @cristianoc

[DZakh]

I find this more reliable:

module Works = {
  let a = "foo"
  type props<'a> = {a?: 'a}

  let make = (props: props<_>) => {
    let a = switch props {
    | {a} => a
    | _ => a
    }

    React.string(a)
  }
}

It won't break if there's a prop with a_ name. Even though there's a vary little chance for this.

[mununki]

I find this more reliable:

module Works = {
  let a = "foo"
  type props<'a> = {a?: 'a}

  let make = (props: props<_>) => {
    let a = switch props {
    | {a} => a
    | _ => a
    }

    React.string(a)
  }
}

It won't break if there's a prop with a_ name. Even though there's a vary little chance for this.

Wow, I like it. I guess this could also apply to #6375.

[mununki]

If we have other props besides a, it is probably better off destructuring like it was used to.

[DZakh]

How about:

module Works = {
  let a = "foo"
  type props<'a, 'anotherProp1, 'anotherProp2> = {a?: 'a, anotherProp1: 'anotherProp1, anotherProp2: 'anotherProp2}

  let make = (props: props<_, _, _>) => {
    let a = switch props {
    | {a} => a
    | _ => a
    }
    let {anotherProp1, anotherProp2} = props

    React.string(a)
  }
}

[mununki]

How about:

module Works = {
  let a = "foo"
  type props<'a, 'otherProp1, 'otherProp2> = {a?: 'a, otherProp1: 'otherProp1, otherProp2: 'otherProp2}

  let make = (props: props<_, _, _>) => {
    let a = switch props {
    | {a} => a
    | _ => a
    }
    let {otherProp1, otherProp2} = props

    React.string(a)
  }
}

I still prefer this: #6374 (comment)

[DZakh]

What I like in this case #6374 (comment) that it also allows to use the props object directly, which might be useful sometimes.

[mununki]

Actually, there is no props in res expession. Exposing a variable called props to the returned element function body may have other side effects.

[mununki]

There is no props in user land.

module DoesntWork = {
  let a = "foo"
  @react.component
  let make = (~a=a) => React.string(a)
}

[DZakh]

I think props is cleaner than a_ and might even be presented as a feature.
Also, I like that in the generated code the argument now called props instead of param: https://rescript-lang.org/try?version=v10.1.2&code=LYewJgrgNgpgBAdRAJwNYGc4F44G8BQccsALnAIbZwBEAZiCNYXCQJ4AO87yI76APAHJyAGjjCAdiBIALGMgAKPdgEYxk6XMXKATAD4qBIkXIB+AFzjRzE1NnylvFZY33tTkTYp2tj9jpdyHwddTyIAX3xmUjhgclR4HAAKbl50S1S+fgB9MVy4bL0ASmwDI2MYyhx0AHcASxIAYxk4TMxyogAfPHJw0oovbuz+8i9I42IYMlwgzRCPbzn3fz6cNqiJgCl0ADooEABzJNm3PxUir229w+Pg5Z0LrwAlGHJGkh30EmQ6iSPyC4RfCRfCgSCwRAoDA6QzRKYUKh0BhMIhsTitZQCYTqE6+ZRqKx3Pz6WETMyBMLGXHzVSBIn4ym2JbEunM0LMEFEGJxBJUJIzSymcj5anLAmi4l5cIZTE5Yr9DpwAD0SooUDq5EwlE1FGyXkqVFqDWaurwXhVcGQMFo8lNWhggzgAGUQMAYMcSlgDKMJt0AHIgCSJb1jDbGK77f70pyA8O7SO3Nm8B5hogvN4fL4-P4ejnA-BAA

[mununki]

Exposing a variable called props to the returned element function body may have other side effects.

I don't think this is a topic of whether the code is clean or not. What do you think about variables called props being exposed in the function body that user didn't express?

[DZakh]

What do you think about variables called props being exposed in the function body that user didn't express?

I find it fine and that the behaviour can be expected. The same as the props type is available even though it wasn't defined.

[DZakh]

Well, I've changed my mind :)

[mununki]

Adding type props is kinda inevitable I think, not like the value props.

[DZakh]

Well, I've changed my mind :)

I've remembered a bunch of React code with props.something in it. It's much better to use destructuring to prevent it

[DZakh]

Well, a good solution would be an ability to set the default value during distructuring, but it's not supported in ReScript:

module Works2 = {
  let a = "foo"
  type props<'a, 'anotherProp1, 'anotherProp2> = {
    a?: 'a,
    anotherProp1: 'anotherProp1,
    anotherProp2: 'anotherProp2,
  }

  let make = ({a=a, anotherProp1, anotherProp2, _}: props<_>) => {
    Js.log(anotherProp1)
    Js.log(anotherProp2)

    React.string(a)
  }
}

[DZakh]

And probably it won't be possible to add support for this until there's the Caml_option.valFromOption

[DZakh]

Here's another suggestion. Although, I can't say that I like it more

module Works = {
  let a = "foo"
  type props<'a> = {a?: 'a}
  %%private(
    let _aWithDefault = _a => {
      switch _a {
      | Some(a) => a
      | None => a
      }
    }
  )

  let make = ({?a, _}: props<_>) => {
    let a = _aWithDefault(a)

    React.string(a)
  }
}

https://rescript-lang.org/try?version=v10.1.2&code=LYewJgrgNgpgBAdRAJwNYGc4F44G8BQccsALnAIbZwBEAZiCNYXCQJ4AO87yI76APAHJyAPiq5yAfgBccYQF9mAUiXcAlgDdyJGAApmRUnAD65BGpIALACIxa5aGRynsYgkQ9x0AdwsBjSxNKd084AB84AGUQYD1yAEpXCgMPCIA5EAA7eCwxchSiRU8iuHj8ZiNgclQcuF1cSXIAGhN5WW5eAWMRRNy8FIB6AYooNXJMSnGKYxSjSmczCxs7BygSXQTyzwAlGHI-EgA6dBJkNUyAcw2ywvxFIA

[cristianoc]

Maybe we can fix this using alias the props name:

module DoesntWork = {
  let a = "foo"
  type props<'a> = {a?: 'a}

  let make = ({a: ?a_, _}: props<_>) => { // alias a as a_
    let a = switch a_ { // refer a_ here
    | Some(a) => a
    | None => a
    }

    React.string(a)
  }
  ...
}

This seems fine.

In general, I would keep changes minimal at this stage and make sure not to introduce possible breaking changes, e.g. to the location shown in error messages, or error messages themselves. As from past experience it's quite easy to introduce issues inadvertently.

[DZakh]

Maybe we can fix this using alias the props name:

module DoesntWork = {
  let a = "foo"
  type props<'a> = {a?: 'a}

  let make = ({a: ?a_, _}: props<_>) => { // alias a as a_
    let a = switch a_ { // refer a_ here
    | Some(a) => a
    | None => a
    }

    React.string(a)
  }
  ...
}

This seems fine.

In general, I would keep changes minimal at this stage and make sure not to introduce possible breaking changes, e.g. to the location shown in error messages, or error messages themselves. As from past experience it's quite easy to introduce issues inadvertently.

Agree, but I'd vote for underscore in front of the var name instead of at the end.

[cristianoc]

Definitely underscore before.
Is it still possible to capture something by accident?