Update handlers API

[alex35mil]

Currently, field update handler takes function of type input => input. Mainly, b/c it guarantees that input value is not stale. But introduces unfortunate caveat:

// Runtime error
onChange={event => {
  form.updateEmail(input => {
    ...input,
    email: event->ReactEvent.Form.target##value,
  });
}}

This runtime error happens due to React's SyntheticEvent being pooled. Since callback gets triggered asynchronously, by the time it gets called, the event is already null'ed by React.

Options

1. Leave it as is.

update[Field]: (input => input) => unit

onChange={event => {
  let value = event->ReactEvent.Form.target##value;
  form.updateEmail(input => {...input, email: value});
}}

Pros: guaranteed that input value is not stale.
Cons: possible runtime error due to event pooling.

2. Change api so it takes just input:

update[Field]: input => unit

onChange={event => {
  form.updateEmail({
    ...form.input,
    email: event->ReactEvent.Form.target##value,
  });
}}

Pros: No event pooling issue.
Cons: possibility of staled input.

3. Expose 2 handlers:

update[Field]: input => unit
update[Field]WithInput: (input => input) => unit

Pros: It's possible to choose a way to update input.
Cons: Bigger API surface and possibility to introduce both issues in user code.

[johnhaley81]

I think that option 2 is my least favorite. You can get into situations where you have invalid state and there is no good work around for it.

I see your point about keeping the API as small as possible. If that's the goal then option 1 seems the best.

I have seen in a few libraries the type of pattern in option 3. Where the user has a base function that handles the normal case and a more powerful function for the not-so-normal cases.

The state being available usually needed unless you have some async stuff going on where while you are updating a field and an async callback comes in with stale data which nukes the changes you just made.

[alex35mil]

Oh I forgot one more option we discussed in PR that's prolly the best of both worlds.

We can add optional flag to ppx:

["re-formality/ppx", "--target=react-dom"]
["re-formality/ppx", "--target=react-native"]

And default it to react-dom.

Then API would be:

// Defined in Formality
type updatePayload = {
  value: string,
  checked: bool,
};

// Application
<input onChange={form.updateEmail(({value}, input) => {...input, email: value})} />

[johnhaley81]

Could that be at a per file level? We have a monorepo with both react native and web apps which would cause is to use this in one of the other but never both.

[alex35mil]

What about environment variable FORMALITY_TARGET?

[johnhaley81]

We compile everything together so a flag like that would be about the same for us. Could we do something like:

module DomForm = [%form { target: dom } ... ];

module NativeForm = [%form { target: native} ... ];

[alex35mil]

Just checked, should be possible!

We can use environment variable FORMALITY_TARGET that would affect all modules. So if a project targets only ReactNative, it wouldn't be required to add local pragma everywhere:

And local override:

module X = [%form
  {target: ReactNative};
  type input = {a: string};
  type output = {a: int};
];

---

PPX flag vs environment variable: I don't use ReactNative so question. Is it common case to have package with Component that can be used in both ReactDom and ReactNative? If yes, then environment variable seems more flexible, b/c I can compile 2 builds for different environments using shared package w/o touching bsconfig.

[johnhaley81]

We do have some components that do cross over the web/native boundary. An environment variable with a local override sounds like a pretty nice flexible solution.

[alex35mil]

@johnhaley81 And one more question: is there a pooling issue in react native?

Usually, 2 things are useful from event target: value and checked (correct me if I miss something). With current reason-react I don't need to know if target is checkbox or not: I can relatively safely extract value and checked from event target and expose both to the handler.

But if in react-native input components expose strongly typed events, then to provide either value or checked to handler I need to know what type of event handler will be dealing with which would require additional annotations all over the place. It's very unfortunate and most likely doesn't worth it. But if pooling is not an issue in react-native, then I can leave things as is in this environment and expect only input => input in react-native update handler.

[alex35mil]

Or maybe I can expose event.target itself on the web (so I don’t need to extract things from it in abstraction) and not sure what in react-native...

[alex35mil]

Looks like event is not required at all in react-native.

[johnhaley81]

@alexfedoseev I'm not sure if there is a pooling issue in react-native. CC @endlo @maxkorp

[alex35mil]

My assumptions are based on:

• https://reactnative.dev/docs/textinput#onchangetext
• https://reactnative.dev/docs/switch#onvaluechange

But there are similar handlers that receive events, so I'm not sure.

[alex35mil]

I.e. if only next value is required, then mentioned handlers can be used. But if developer uses event-based handler, then it means that there's something else needed from event and developer would be extracting stuff anyway, so it makes sense to keep input => input on react-native target.

[alex35mil]

Also, does it makes sense to change blur handler so it accepts event as well for consistency?

<input
  onBlur={form.blurEmail}
  onChange={form.updateEmail((input, target) => {...input, email: target##value})}
/>

[alex35mil]

Implemented in #72

[alex35mil]

We faced the case when implemented change didn't work. The current update handler expects that event is always of ReactEvent.Form.t but it's not always the case. E.g. when a state gets updated via some fancy UI control (like DatePicker), an event might be of another type (ReactEvent.Mouse.t or ReactEvent.Keyboard.t etc). So guessing we will need to update the current interface so update handler would be of ReactEvent.Synthetic.t.

<input
  onBlur={form.blurEmail}
  onChange={
    event =>
      form.updateEmail(event->ReactEvent.toSyntheticEvent, (input, target) =>
        {...input, email: target##value}
      )
  }
/>

/cc @johnhaley81

[alex35mil]

I'm not sure that there can't be the case when there is no event provided at all. E.g. some custom component exposes just string. In this case, event capturing wouldn't work.

[alex35mil]

Unfortunately, we should revert #72.
Antd has a number of components that expose only value, e.g.

https://ant.design/components/slider/
https://ant.design/components/date-picker/

[johnhaley81]

@alexfedoseev looking at how different components can be passing back values, I think assuming a specific type of value is coming back is probably a misstep. While putting {target: ReactNative} is a work-around, that's not a good DX.

I think reverting #72 is a good move unfortunately.

Instead of assuming the type of value coming in, we could have the updater functions be in the form of:

let updateField: ((`state, `field) => `state, `field) => unit;

This would allow re-formality to be used anywhere for any component since we only care about getting the type of the field which is as small of a requirement as possible.

Additionally, we could provide some utility functions that make getting values from DOM elements consistent. Something with functions like:

let getValueFromEvent: ReactEvent.Form.t => string =
  event => event->ReactEvent.Form.target##value;

let getCheckedFromEvent: ReactEvent.Form.t => bool =
  event => event->ReactEvent.Form.target##checked;

Which could be composed with the updateField function using some glue function like an andThen function which is commonly made into an inline operator >>.

let (>>) = (g, f, a) => g(f(a));

What it looks like all together:

<input
  onChange={
    getValueFromEvent >> form.updateEmail((form, email) => {...form, email})
  }
/>

And with components that don't directly give you access to the DOM it would be pretty simple.

<DatePicker onChange=form.updateDate((form, date) => {...form, date}) />

[alex35mil]

@johnhaley81 Sorry for the delay, passing in value makes sense and it's done in #78. Can you review it, please?