[WIP] Cleanup codebase from Js.t thanks to bs-platform@7 object as records by MoOx · Pull Request #631 · rescript-react-native/rescript-react-native

Will closes #629 when finished.

Alain Armand and others added 3 commits

December 12, 2019 12:06

I just made some changes converting Js.Nullable.t to option. The code compiles but I am not sure about it. How do we go about this. Do I ask for a review? Also, how have you been testing your code?

Note that some of the .rei files are showing warnings about unused bs.string attributes. Of course is you take them out the code doesn't compile. I am thinking this is an editor thing.

Thanks.

@idkjs Please be careful regarding Js.Nullable.t. The option value None only covers undefined, not null. If an API really returns a null value, replacing Js.Nullable.t with option is incorrect and will lead to crashes/unpredictable behavior.

So you would need to check each case individually to be sure it is safe and the API never returns null before converting it to option.

		title: option(string),
		message: option(string),
		tintColor: option(Color.t),
		};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only values returned from an API (e.g. as callback params) should be converted to records. Values passed to an API (options) should not be converted to records, because records cannot have optional fields.

With the current bindings, you can do

let options = ActionSheetIOS.options(~options=[|"A", "B", "C"|], ());

(all the other props being optional). If converting to records you won't be able to do

let options = { ActionSheetIOS.options: [|"A", "B", "C"|] };

but would have to supply all the remaining props with value None which is very inconvenient.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, we should be using records where we are currently using Js.t objects, but not where we are currently using abstract types.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you would need to check each case individually to be sure it is safe and the API never returns null before converting it to option.

I am happy to do this. How would you go about doing that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cknitt: You can create the same constructor function (that currently creates objects) to create records with default value None.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfrolich Can you show an example to be sure about what you mean?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi folks. Sorry for the prolonged absence. I am back now. If I am reading this correctly, we were considering converting our input types back to bs.obj's. Do we still want to do this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great you are back @idkjs! I think it could be records unless input API's rely on existence of keys.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that using the record syntax gets requires the following option code:

let options: ReactNative.ActionSheetIOS.options = {
  options: buttons,
  cancelButtonIndex: Some(4),
  destructiveButtonIndex: Some(3),
  tintColor: Some("green"),
  title:None,
  message:None
};

Which compiles to

var options = {
  options: buttons,
  cancelButtonIndex: options_cancelButtonIndex,
  destructiveButtonIndex: options_destructiveButtonIndex,
  title: undefined,
  message: undefined,
  tintColor: options_tintColor
};

and runs fine. Don't know if this answers the question. I think going with records is the way to go for what it is worth. At the very least, you will always be aware of what the js code is doing. Examples running against bs.obj idkjs/ActionSheetIOSDemo. Against records in a minute.

using records syntax can be seen on the idkjs/ActionSheetIOSDemo/#using-records branch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you folks want to do with this? bs.obj or records @MoOx @cknitt? @jfrolich is for records from what I see.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we definitely don't want to type

let options: ReactNative.ActionSheetIOS.options = {
  options: buttons,
  cancelButtonIndex: None,
  destructiveButtonIndex: None,
  tintColor: None,
  title:None,
  message:None
};

When you can just ask people to write this

let options = ActionSheetIOS.options(~options=buttons, ());

So we must keep bs.obj for now.

MoOx mentioned this pull request

Jan 17, 2020

@idkjs are you still motivated to continue the effort here or do you want help with this?

@MoOx I am motivated. Saw a message where you said you had forked and merged. Thought you were done. Confused about what happened here.

I am in. I need to review the comments and get back on it. Sorry for the misunderstanding

No problem. I just created this PR. I think I merged some commits into this repo but it's not "in master" yet (hence, this PR).

You can push commits on this branch, you should have the proper rights :)

Ok. Will get back on this. Thanks for the heads up.

Regarding these conflicts, Its just a question of deciding which way to go, bs.obj or records, correct? In the mean time, I will resolve them back to bs.obj until a decision is made.

@jfrolich I'm for records. The discussion is about which way to go, isn't it?

Consider the Style.t type. If we go for record, each style definition will have 101 keys, the great majority with value undefined. Unless some key must exist, even if it's null or undefined, I think bs.obj is the cleaner solution.

Worse yet, we'd lose the bs.string decorator and have to resort to bs.inline. Having to use the latter for return values is painful enough, I don't think we want that for something like Style.t for example.

Consider the Style.t type. If we go for record, each style definition will have 101 keys, the great majority with value undefined. Unless some key must exist, even if it's null or undefined, I think bs.obj is the cleaner solution.

Worse yet, we'd lose the bs.string decorator and have to resort to bs.inline. Having to use the latter for return values is painful enough, I don't think we want that for something like Style.t for example.

Agree. I think at some point Bucklescript can add more functionality to also have this use case covered for records. For style the type is an implementation detail and not user-facing anyway so it doesn't really matter.

Agree. I think at some point Bucklescript can add more functionality to also have this use case covered for records. For style the type is an implementation detail and not user-facing anyway so it doesn't really matter.

@jfrolich what do you mean here by implementation detail? And the fact that it is not user facing means that we can use records because the end user wont ever see it?

Sorry, I meant that even when Style.t is still a Js.t it's not user facing, because for the user it's an opaque type. So imo not really necessary to make it a record type if there are some downsides to it at the moment (much unnecessary generated code).

Too lazy to review all this thread. What is the status of this PR?

I am willing to give a helping hand to push this forward. I think @cknitt makes a compelling case of keep using [@bs.obj] for objects passed to react-native.

@Arnarkari93 feel free to make another PR & bump to bs-platform 7.2 :)

I resolved the conflicts to make sure we do not add textInputEvent back into the repo. I've also chosen to remove changes to yarn.lock that were due to an early v7 release of bs-platform. Any such dependencies should be updated with a newer release anyway.

Closing in favour of #666 that looks more promising. Thanks for everyone involved in this WIP.

MoOx deleted the bs-platform-7.0.1 branch

November 11, 2020 06:31