Adaptations for ReScript 10 / use records with optional fields

[cknitt]

Closes #766. Wait for ReScript 10 to be released before merging.

Trying to remain compatible as far as possible by keeping (but deprecating) existing creation functions (@obj).

[cknitt]

Updated PR for 10.0.0 release version.

[MoOx]

@cknitt can you check my latest commits please ?

[cknitt]

Looks good to me.

For shareResult and interactionState there is little benefit in having optional fields, as it is unlikely that the user will ever construct instances of these record types himself. However, they won't hurt either, so I'd say let's go for it to be consistent with the other records definitions.

[MoOx]

Btw, I think I read somewhere that type record spread is coming, it this a thing ?

[cknitt]

Yeah, I pinged you in https://github.com/rescript-lang/rescript-compiler/issues/5659. 😄

There is a PoC now: https://github.com/rescript-lang/rescript-compiler/pull/5715

[cknitt]

I guess a codemode would be nice but I don't feel confortable handling it. Never did one and don't have much time right now. But what if?... We temporarily keep array, arrayOption & style functions (but deprecate them) and adjust their result... since RN handle nested array !?

• style() could return a array (here we use external + a tiny function ?)
• array/arrayOption could accepts array & return styles too ?

This could work and would avoid people a major breaking change ? Am I missing a thing ?

What we can do is have Style.style(), Style.viewStyle() etc. return Style.styles = Style.array<Style.t> as you suggested. This can then be passed to a view's style prop as before.

But

external array: array<styles> => styles = "%identity"
external arrayOption: array<option<styles>> => styles = "%identity"

is equivalent to

external array: array<array<Style.t>> => array<Style.t> = "%identity"
external arrayOption: array<option<array<Style.t>>> => array<Style.t> = "%identity"

which are not correctly typed. This was ok with an abstract type as a result, but is not correct anymore now. I it will work though, as long as the user only uses the resulting value to pass it to a view's style prop.

[MoOx]

For array & arrayOption, we could turn them into a function instead of external ?

[cknitt]

Yes, but this has performance implications.

[MoOx]

The idea is still to have them deprecated. Do you think that's a bad idea to make them ?

[cknitt]

No, I can do that. 👍

[cknitt]

BTW, I am wondering about the general performance implications of passing an array instead of a style object now.

The style object would usually be created once (StyleSheet.create) and then always the same instance would be passed. When wrapping it in an array, this will be a new array instance every time.

[MoOx]

That's right. I never measured this. I use array everywhere but never thought about it. @Freddy03h are you doing the same, array inside jsx or do you compute them first ?

I generally care about perfs when I "feel" that there is an issue, but having this everywhere could have an impact but I have no clue if that could be feel by the end user or not 😁

[cknitt]

So this will run for every style unnecessarily passed within an array:

https://github.com/facebook/react-native/blob/73bcedb903c330fddca1cab40bc501cc073a4872/Libraries/StyleSheet/flattenStyle.js#L28

We are really moving away from the zero cost idea here, I am getting second thoughts again. 😞

[MoOx]

In practise for me this won't change a thing as I use array almost everywhere. But I totally get your point. Assuming that's a thing, can we measure this ? If you have a complex screen with, let's say, hundreds of style declaration that have a single item, what is the impact per render in terms for performances ? Is this >1ms, >10ms, >100ms ?

[Freddy03h]

I don't think it will be a performance issue.

But… I'm thinking… array was just a convenient way of merging style prop, in a time spread syntax wasn't easily usable

There is no difference between these in js :

style={[style, styleFromProp, { paddingTop: insets.top }]}

style={{ ...style, ...styleFromProp, ...{ paddingTop: insets.top } }}

So it could be possible to completely ditch the array!

For now it's not seems to be possible with rescript record… But the error isn't relevant anymore since optionnal fields :

Records can only have one ... spread, at the beginning. Explanation: since records have a known, fixed shape, a spread like {a, ...b} wouldn't make sense, as b would override every field of a anyway.

[MoOx]

@Freddy03h using array or object is imo approximately the same result: on every render, you "change" styles. Compared to using a stylesheet (which create a "static" object) where you never change the style prop.

This don't work for dynamic styles as you always have to "get" the theme or dynamic base from somewhere in your app.

In short: for people like you and me, approximately no diff. For people using StyleSheet.create only: maybe a change on runtime, but should be measured... If you don't re-render a lot, probably a tiny change.

[Freddy03h]

@MoOx It's also possible to open a PR on react-native to optimize flattenStyle for an array of 1 if we found perf issue

But what are you thinking about ditching the array?

[cknitt]

@Freddy03h Actually ditching the array and just spreading would be nice, but the language support is not there yet.

Also, you wrote:

I read that it could be back to int (or other value) in react-native-web with the new/next style system

If that's the case, then the style values won't be records and the spreading is not going to work.

[Freddy03h]

😅

…

For the performance issue: to keep an usable app on low end device with Android 6, you need to memoize your components if there is too much rerenders. So, I think the problem with flattenArray would be an issue only if there is too many re-render of your component, and in these case you would need to memoize it anyway.

[cknitt]

@MoOx @Freddy03h I have given this more thought over the weekend.

There are various third party libs out there for rescript-react-native. If we change Style.t to be a record and all style props to accept Style.styles = array<Style.t> instead of Style.t like in my last commit, then all such libs would have to adapt to that, too. I think that would be too much breakage, and we should stay with Style.t being the type used by the style props.

For me there are still too many uncertainties / moving parts here. Some concerns may be mitigated later though (when extensible records are available in ReScript so that components from third party libs can inherit their base props from components in rescript-react-native). Not so sure about others, like react-native-web maybe using ints for styles.

For now, I would propose to finish this PR without the change to arrays for style props, i.e. still keeping Style.t abstract as it is now, and continue the discussions about arrays for style props in a separate issue.

I would simplify things though so that there is a single type for style records (not viewStyleObj, textStyleObj etc.) and that it is used like I described above (with just s instead of style):

let styles = {
  open Style
  StyleSheet.create({
    "playButton": s({
      display: #flex,
      alignItems: #center,
      justifyContent: #center,
      borderWidth: StyleSheet.hairlineWidth,
    }),
  })
}

And people can still use the existing style() function as an alternative.

BTW, I was also thinking that maybe we should not deprecate the old creation functions yet, but wait for that until a later release to give users some time to transition.

[cknitt]

@MoOx Updated the PR as described. Can we get it merged and continue discussing/evaluating the array style props ideas elsewhere?