isprojects / mstform

Mobx State Tree Form-library
MIT License
81 stars 3 forks source link

React Native support #159

Open kav opened 3 years ago

kav commented 3 years ago

Great library! Any thoughts on React Native support? Happy to take a stab at it though it's probably not the best first issue.

faassen commented 3 years ago

Since mstform takes care of form state but not rendering it should just work on React Native as long as mobx-state-tree works.

On Sun, Oct 10, 2021, 05:11 Kavanaugh Latiolais @.***> wrote:

Great library! Any thoughts on React Native support? Happy to take a stab at it though it's probably not the best first issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/isprojects/mstform/issues/159, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACP6MDGCZPXIVI3KIUGMYLUGD74RANCNFSM5FV7AUBQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

kav commented 3 years ago

It almost does. I can post more details and an expo snack when I'm back on my laptop shortly and start outlining the issues

I think the biggest issue is the inputProps the field provides don't quite match the RN version. I believe it's the onChange handler having a slightly different format on the event object leading to a can't call trim() of undefined.

kav commented 3 years ago

Here's the snack https://snack.expo.dev/@kavla/mst-forms-playground Note it works fine on React Native Web, likely due to the underlying <input> but if you launch it on one of the Appetize.io devices or your own it'll fail with an e.trim of undefined error upon text entry.

kav commented 3 years ago

After debugging in the iOS simulator it looks like that is indeed the issue https://github.com/isprojects/mstform/blob/master/src/controlled.ts#L10 ev.target.value is undefined in RN. Something like #160 could fix it. Very draft as I've done no testing beyond debugger inspection there just starting the convo around fix arch. I can look at adding RN Jest to the party if you'd like. Might even flush out some more issues.

faassen commented 3 years ago

Have you looked at this? https://github.com/isprojects/mstform#controlled-props

faassen commented 3 years ago

In the PR you changed the controller. Currently the converters assume things about the default controller, but you could create your own React Native controller and use that explicitly.

It would be interesting to consider whether we can explicitly define the mapping it should use so that in React native the defaults are the right thing, but I think custom controllers would work for now.

RickLucassen commented 2 years ago

Hey @kav , how did things work out for you? Could you make everything work by defining your own controllers?

Basically what I'm getting from your PR is that you made it work by using onChangeText instead of onChange. As I'm reading the docs of react-native I see that the normal onChange should also work: https://reactnative.dev/docs/textinput#onchange

But somehow the event.target.value is undefined in the case of react-native?

kav commented 2 years ago

Yeah the initial problem is that onChange in ReactNative doesn't have a target because the SyntheticEvent isn't there. https://reactjs.org/docs/events.html Now you could use the e.nativeEvent.text generally, including in regular React and that's also where I started but I was concerned that might have repercussions for React cases I wasn't testing. See https://github.com/isprojects/mstform/pull/160/commits/cca39ee0482e4d12fce05c1fa3720fb8ca358acc Probably could just fallback if e.target isn't there rather than the reverse.

Just using e.nativeEvent.text in place of e.target.value I'm fairly certain will work for all cases but might need a bit of cross browser testing to confirm. I figured a PR that doesn't pass through that case might be easy to accept but as you note it's ideal if they can all use that same codepath.

I've been running with this PR patched in for the time being as it's a relatively simple fix but adding a controller and checking runtime environment is on my list somewhere.... ;)

So far it's gone pretty well all thing considered.

RickLucassen commented 2 years ago

@kav I looked at this further and using nativeEvent seems to be working. I've put together some changes in this branch: https://github.com/isprojects/mstform/commits/react-native-support

This does work within react although some extensive testing needs to be done. According to this: https://developer.mozilla.org/en-US/docs/Web/API/Event/target the .target on an Event is widely supported. So I don't see any issues with that. A quick test within a react-native "snack" also shows this should work, could you maybe test to see if this would work?

kav commented 2 years ago

Cursory look it appears that the native event in react native has a structure like:

{ 
  eventCount: 1,
  target: 134,
  text: 'hello world',
}

so still not working since target is just an int. If you've got a snack I can do some comparison to see if I can resolve that

RickLucassen commented 2 years ago

I've extended your snack with an example: https://snack.expo.dev/R1Yk-vdb6

Indeed it doesn't seem to work with react-native. The preferred solution would be to use onChangeText for react-native as it seems, however this wouldn't work for regular react applications.

The only solution seems to be to create either something to make mstform configurable and be able to set whether you're working with react or react-native or add checks to see if something is undefined and check another property at that point.

Not really sure yet at this point.