Adaptations for ReScript 10 / use records with optional fields #769

cknitt · 2022-08-15T01:08:30Z

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).

MoOx

Awesome work ! Thanks @cknitt.
I think we could improve styleObj being directly t so we don't even have to use styleObj function but beside this, it's perfect !

package.json

src/apis/Style.res

cknitt · 2022-08-28T13:27:23Z

Updated PR for 10.0.0 release version.

* main: "end" is not a reserved word anymore 0.69.1 Fix 0.69 not compiling on ReScript 10 (#773) 0.69.0 Create HitSlop & Rect module (closes #765)

@cknitt

@cknitt can you check if everything I added is correct (especially for VirtualizedSectionList, not sure if that make sense to change as record...) ? Thanks :)

MoOx · 2022-10-05T11:01:39Z

@cknitt can you check my latest commits please ?

cknitt · 2022-10-05T11:21:46Z

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

Everything is amazing, but I have concern for Style API changes.

MoOx · 2022-10-05T13:41:05Z

src/apis/Style.res

 // Contains all of
 // - image style
 // - text style
 // - view style
 // - transform style
 // - shadow style
 // - layout style
+type styleObj = {


Just a question here for everyday need:

Now we are supposed to do style={Style.styleObj({width: 10.->Style.dp})} instead of style={Style.style(~width=10.->Style.dp,())}. Is there really a direct benefit ? Because with style being styleObj, at the end, we don't save a single char :D
Could we imagine something shorter ? Or maybe directly a record ? I know that array would be an issue but... What if we only accept array as discussed in #632 ? This way, we could only type style={[{width: 10.->dp}]}.
I have to admit that with themed app (light/dark mode) and predefined common styles, I have most of the time use array.

And it would be like React.useState() for example. Original api accepts value or function, but rescript bindings only accept the "most advanced" to avoid bloated api. By using array, we directly allow us to save chars everytime !

Any thoughts ? cc @Freddy03h

Yes, no ~ anymore, no () at the end anymore that you can forget, more readable and more familiar to JS developers, definitely a benefit in any case.

But it would certainly be nice to be able to avoid Style.style/Style.styleObj altogether.

I don't see how it could work in a type-safe manner though, given that nested style arrays are also allowed (and sometimes useful), and that we may also want to keep arrayOption. I think we do need our abstract type (Style.t).

Maybe we should just make it s instead of styleObj. Example:

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

I will have to remove this then:

@module("react-native") @scope("StyleSheet") external flatten: array<Style.t> => Style.t = "flatten"

let's break all the things. It's been a while now that StyleSheet.create doesn't returns int anymore but directly styles. It's only validating some values & units.

Done!

With all these breaking changes I guess we'll need a codemod?

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

MoOx · 2022-10-05T13:49:06Z

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

@int

* main: Use polymorphic variant for Platform.os (#771) Get rid of @string/@int for polyvars (#770)

cknitt · 2022-10-05T14:27:15Z

Yeah, I pinged you in rescript-lang/rescript#5659. 😄

There is a PoC now: rescript-lang/rescript#5715

MoOx

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 ?

MoOx · 2022-10-07T04:38:48Z

src/apis/Style.res

+  switch style {
+  | Some(style) => style
+  | None => empty
+  }


Can't we use identity to do option => t ?

No, because t is a record type now, but None is not a record.

Yeah I get that. I was just thinking that RN styles accepts undefined, null & falsy values...

Yeah but the user may do

let x = Style.optional(None) let color = x.color // 💥 runtime exception

Yeah right, without abstract type, that's mandatory :)

MoOx · 2022-10-07T04:41:30Z

src/apis/StyleSheet.res

@@ -7,5 +7,3 @@ external absoluteFillObject: Style.t = "absoluteFillObject"

 @module("react-native") @scope("StyleSheet")
 external create: Js.t<'a> => Js.t<'a> = "create"
-@module("react-native") @scope("StyleSheet")
-external flatten: array<Style.t> => Style.t = "flatten"


I am wondering if this can still work per RN docs. It's supposed to handle exactly this: flatten an array. Since we type styles != type t, this could still work right ?

Ah, I think you are right, I was confusing it with "%identity", this should still work as it is. So I can add it back.

But you won't be able to directly pass the result directly to a view's style prop anymore (as that expects an array now).

cknitt · 2022-10-07T05:47:49Z

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 · 2022-10-07T05:52:45Z

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

cknitt · 2022-10-07T05:54:49Z

Yes, but this has performance implications.

MoOx · 2022-10-07T05:56:23Z

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

cknitt · 2022-10-07T05:57:01Z

No, I can do that. 👍

cknitt · 2022-10-07T05:57:06Z

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 · 2022-10-07T06:00:05Z

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 · 2022-10-07T06:13:20Z

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 · 2022-10-07T06:23:26Z

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 · 2022-10-07T16:03:51Z

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 · 2022-10-07T16:09:12Z

@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 · 2022-10-07T16:17:36Z

@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 · 2022-10-07T16:33:16Z

@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 · 2022-10-07T22:57:59Z

😅

…

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 · 2022-10-10T15:03:02Z

@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 · 2022-10-11T16:54:56Z

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

MoOx

Yeah looks good. You are right that this need some thoughts.

Get rid of @string/@int for polyvars (#767)

f078bb5

cknitt requested a review from MoOx August 15, 2022 01:08

MoOx mentioned this pull request Aug 18, 2022

Easy syntax for stylesheet without Js.t #667

Closed

MoOx reviewed Aug 18, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/apis/Style.res Outdated Show resolved Hide resolved

MoOx added this to the 0.70 / rescript 10 milestone Aug 18, 2022

MoOx force-pushed the main branch from f078bb5 to 4f04d77 Compare August 18, 2022 09:21

cknitt force-pushed the rescript-10 branch from 1a604da to 59ebb51 Compare August 28, 2022 13:26

ReScript 10

78439fd

cknitt force-pushed the rescript-10 branch from 59ebb51 to 78439fd Compare August 28, 2022 13:26

cknitt marked this pull request as ready for review September 15, 2022 17:13

MoOx added 3 commits October 5, 2022 12:42

Update package-lock.json

32847cf

Merge branch 'main' into rescript-10

cd7687b

* main: "end" is not a reserved word anymore 0.69.1 Fix 0.69 not compiling on ReScript 10 (#773) 0.69.0 Create HitSlop & Rect module (closes #765)

add some missing conversion

adc16c4

@cknitt can you check if everything I added is correct (especially for VirtualizedSectionList, not sure if that make sense to change as record...) ? Thanks :)

Update package-lock.json

755aad1

MoOx reviewed Oct 5, 2022

View reviewed changes

Merge branch 'main' into rescript-10

443e365

* main: Use polymorphic variant for Platform.os (#771) Get rid of @string/@int for polyvars (#770)

MoOx previously approved these changes Oct 7, 2022

View reviewed changes

MoOx force-pushed the main branch from 316c73c to 036566e Compare October 7, 2022 05:30

Simplify style module and undeprecate old style creation functions

f645128

cknitt dismissed MoOx’s stale review via f645128 October 11, 2022 16:53

cknitt force-pushed the rescript-10 branch from a536606 to f645128 Compare October 11, 2022 16:53

MoOx approved these changes Oct 12, 2022

View reviewed changes

MoOx merged commit e606a03 into main Oct 12, 2022

MoOx deleted the rescript-10 branch October 12, 2022 07:30

cknitt mentioned this pull request Oct 21, 2022

Get ready for releasing 0.70.0 #779

Closed

5 tasks

Uh oh!

Adaptations for ReScript 10 / use records with optional fields #769

Adaptations for ReScript 10 / use records with optional fields #769

Uh oh!

Conversation

cknitt commented Aug 15, 2022

Uh oh!

MoOx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cknitt commented Aug 28, 2022

Uh oh!

MoOx commented Oct 5, 2022

Uh oh!

cknitt commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoOx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoOx commented Oct 5, 2022

Uh oh!

cknitt commented Oct 5, 2022

Uh oh!

MoOx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoOx commented Oct 7, 2022

Uh oh!

cknitt commented Oct 7, 2022

Uh oh!

MoOx commented Oct 7, 2022

Uh oh!

cknitt commented Oct 7, 2022

Uh oh!

cknitt commented Oct 7, 2022

Uh oh!

MoOx commented Oct 7, 2022

cknitt commented Oct 5, 2022 •

edited

Loading

cknitt Oct 5, 2022 •

edited

Loading

cknitt commented Oct 7, 2022 •

edited

Loading

Freddy03h commented Oct 7, 2022 •

edited

Loading

Freddy03h commented Oct 7, 2022 •

edited

Loading