Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add ReasonReact.listToElement #192

Closed
mimshwright opened this issue Feb 26, 2018 · 14 comments
Closed

Feature: Add ReasonReact.listToElement #192

mimshwright opened this issue Feb 26, 2018 · 14 comments

Comments

@mimshwright
Copy link

Just to simplify the conversion process further.
listToElement : list(reactElement) => arrayToElement(Array.of_list(list))

@cullophid
Copy link
Contributor

Yay

@mimshwright
Copy link
Author

hmm. looks like perhaps this is defined in reason-react/ReactMini/src/React.re but not included in the ReasonReact module? Is that intentional?

@mimshwright
Copy link
Author

Oops, this is a duplicate of #140

@chenglou chenglou mentioned this issue Mar 1, 2018
@chenglou
Copy link
Member

chenglou commented Mar 1, 2018

ReactMini is a small experiment that has not much to do with ReasonReact itself and isn't shipped

@djyde
Copy link

djyde commented Apr 27, 2018

I implement it in this way:

let listToReactElement = (list, fn) =>
  list |> List.rev_map(fn) |> Array.of_list |> ReasonReact.arrayToElement;

let listToReactElementi = (list, fn) =>
  list |> List.mapi(fn) |> Array.of_list |> ReasonReact.arrayToElement;

@chenglou
Copy link
Member

This really kills perf and is not recommended

@djyde
Copy link

djyde commented Apr 28, 2018

So what is the best practice

@chenglou
Copy link
Member

Use array. React children should probably not be modeled as a list

@cullophid
Copy link
Contributor

But react children are a result of rendering application state. So that means changing all the lists in your app to arrays.
By doing so you are sacrificing immutability.
I have seen quite a few newcomers pick arrays over lists just for this reason.

@viktor-izettle
Copy link

Would also be nice with a few more. For example ReasonReact.int. It gets old quickly to always convert all these common types.

@praveenperera
Copy link

praveenperera commented Nov 2, 2018

@chenglou

Use array. React children should probably not be modeled as a list

Does that mean this

[1,2,3]
-> Belt.List.map( num => <div> {num -> string_of_int -> ReasonReact.string} </div>)
-> Belt.List.toArray
-> ReasonReact.array

Is slower than

[1,2,3]
-> Belt.List.toArray
-> Belt.Array.map( num => <div> {num -> string_of_int -> ReasonReact.string} </div> )
-> ReasonReact.array

@IwanKaramazow
Copy link
Collaborator

The problem is that Belt.List.toArray traverses the whole list and adds its elements (one by one) to an array. That extra traversal can be avoided by using plain arrays.

@praveenperera
Copy link

@IwanKaramazow

The problem is that Belt.List.toArray traverses the whole list and adds its elements (one by one) to an array. That extra traversal can be avoided by using plain arrays.

Oh I see, so it might make sense to model the underlying data into arrays instead of lists from the start? For example decoding a list of users from the server, into an array instead of a list.

@IwanKaramazow
Copy link
Collaborator

Yes indeed, modelling everything as array from the start is indeed recommended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants