Fix default values of selected/selectedGlyphImage

[Nicolapps]

See #38 (comment)

[nikischin]

Also I personally feel, from a MapKit JS perspective, I would rather want the implementation to not pass those props which are not defined, than passing them with the default values. Not sure if Apple thinks like I do, though, it could even at some point prevent some errors. Talking about the selected feature for example. Let's say we haven't defined the prop, then the user selects the annotation and then we pass in a false. Would the API be even able to get the changed value, as internally selected is already true, though the prop passed (default value = false) is false and the new passed value also is false. Please correct me if this does not make sense what I am thinking here.

Could we possibly also discuss this here? So basically what I am saying is that it might be better to pass undefined whenever a prop is not set and not pass any prop with the value undefined and just those with a real value, which might be equal to the default value. Please let me know what you think about it or if this does not make any sense.

[Nicolapps]

Sorry, I didn’t see your update when merging your PR. I see the point in your suggestion, but I think that it might cause an issue.

When using boolean attributes that default to false in JSX, it is common to be able to set the value to false by omitting the attribute (<…>) and set it to true by including it (<… selected>). With your suggestion, the component would behave unexpectedly when used like this:

function MyMarker({ isCurrent }) {
  return isCurrent
    ? (
      // In this case, the marker should never be selected
      <Marker
        // (…a lot of styling properties specific to what the user needs in this case)
        enabled={false}
      />
    ) : (
      // In this case, the marker should always be selected
      <Marker
        // (…a lot of styling properties specific to what the user needs in this case)
        enabled={false}
        selected
      />
    );
}

I’m open to making a breaking change if it improves the developer experience, but I’m worried by it behaving strangely in a situation like this one.

[nikischin]

Sorry, I didn’t see your update when merging your PR. I see the point in your suggestion, but I think that it might cause an issue.

When using boolean attributes that default to false in JSX, it is common to be able to set the value to false by omitting the attribute (<…>) and set it to true by including it (<… selected>). With your suggestion, the component would behave unexpectedly when used like this:

function MyMarker({ isCurrent }) {
  return isCurrent
    ? (
      // In this case, the marker should never be selected
      <Marker
        // (…a lot of styling properties specific to what the user needs in this case)
        enabled={false}
      />
    ) : (
      // In this case, the marker should always be selected
      <Marker
        // (…a lot of styling properties specific to what the user needs in this case)
        enabled={false}
        selected
      />
    );
}

I’m open to making a breaking change if it improves the developer experience, but I’m worried by it behaving strangely in a situation like this one.

Thank you so much for your feedback!

And indeed, that is a valid point. Although I personally never use this syntax. However React is taking care of this already as far as I know and does not pass a undefined, but a true instead.

If you would however use

const [selected, setSelected] = useState();

...
return (
<Marker selected={selected} />
)

the value would be undefined. And could be set to false using setSelected(false), for example using button or something.

I know this is quite a edgy case and quite a made up problem, though rather a general question if this could improve the library maybe. From my point of view this is only an issue when making managed props unmanaged (solution already provided) or unmanaged props managed during the life cycle of a component (solution would be to make the default values undefined instead of their MapKit JS default value equivalent and unset undefined values with the solution already provided). If this is too made up and too far from real problems, let's just forget about this ...

[Nicolapps]

However React is taking care of this already as far as I know and does not pass a undefined, but a true instead.

React will pass true when the attribute is set, but undefined (and not false) when omitting it. My point was that in some cases, people could expect omitting the attribute to behave the same way as setting it to false, while the proposed change breaks this property.

I tested how React behaves with checkboxes, because it is a similar case. It turns out that when changing checked from true to undefined, React deselects the checkbox, instead of keeping its current state.

For this reason, I think that it is better to keep the current behavior for selected. As you pointed out, the cases where the attribute is always controlled and the case where it is always uncontrolled are already handled well. Thank you a lot for your proposal and research!

[nikischin]

However React is taking care of this already as far as I know and does not pass a undefined, but a true instead.

React will pass true when the attribute is set, but undefined (and not false) when omitting it. My point was that in some cases, people could expect omitting the attribute to behave the same way as setting it to false, while the proposed change breaks this property.

I tested how React behaves with checkboxes, because it is a similar case. It turns out that when changing checked from true to undefined, React deselects the checkbox, instead of keeping its current state.

For this reason, I think that it is better to keep the current behavior for selected. As you pointed out, the cases where the attribute is always controlled and the case where it is always uncontrolled are already handled well. Thank you a lot for your proposal and research!

Thank you so much for your feedback!

I would like to note two things as a side note to keep in mind. Omitting a value and setting it to false, doesn't behave the same way if set on initial render. While a <input checked={false} /> is a controlled component, <input/> is not. However, yes if you make a controlled component uncontrolled, the behavior you described is indeed like this. Not sure if you wanted to say this already, but just wanted to point out.

Also React does warn when making an uncontrolled value controlled. Not sure if we should consider implementing a similar warning maybe at some point.

Warning: A component is changing an uncontrolled input to be controlled. This is likely caused by the value changing from undefined to a defined value, which should not happen. Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components

Even more precise, React is behaving the exact same way on the inputs as our library currently does. It does actually not set it to false but even resets it to it's defaultvalue. Not saying that I like this behavior, but well I can fully agree on your perspective of staying as close to the React behavior as possible.