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

Crio and React - Array Map #20

Closed
andreigabreanu opened this issue Jul 10, 2016 · 9 comments
Closed

Crio and React - Array Map #20

andreigabreanu opened this issue Jul 10, 2016 · 9 comments

Comments

@andreigabreanu
Copy link

It seems that when using Crio with React and you have an array of objects and try to do a simple iteration crio fails with error (see below).

Adding a .thaw().map() instead fixes the problem so it is probably something related to how React interacts with Crio during the map.

screen shot 2016-07-10 at 15 45 45

@planttheidea
Copy link
Owner

I'll scope this out as soon as I'm home, I'm on vacation right now but will be home this evening.

Can you post the code you used to create this? I want to make sure I replicate the circumstances as best I can.

@andreigabreanu
Copy link
Author

andreigabreanu commented Jul 10, 2016

Unfortunately it is proprietary code so can't copy paste directly. However it is something similar to :

`
export function SomeStatelessComponent( props) {

return (
    <div>
        {props.data.map(( item, offset ) => (
            <SomeOtherComponentw
                key={item.id ? item.id : Math.random()}
            />
        ))}
    </div>
);

}
`

where the props contains a property called "data" which is an object similar to:

const state = crio({UI: {data: []}});

Some additional info:

  1. I am using react-redux + redux and I am injecting the data via the @connect decorator from react-redux. That is how the data arrives in the component
  2. I am using JSX and the jsx-control-statements npm package.
  3. Doing a state.UI.data.map(...) anywhere in the code seems to be fine except when done in a JSX component
  4. It seems that if I remove the return part and just do a simple map w/o any return it works. Adding a return <div></div> seems to create the issue. I find it very weird.

Let me know if I can help more.

@planttheidea
Copy link
Owner

I just reread this again and I have a feeling I know what the problem is... if the item returned from the map is an object or array, it tries to re-crio the returned value into a new crio. This is not necessary when the object returned is a react object.

I'll try to come up with a generic isPlainObject checker so that this doesn't happen with react objects or other non-POJOs.

@andreigabreanu
Copy link
Author

I think it makes sense. I think the other libraries (seamless immutable or freezer not sure) also had this issue and it was quite annoying to have to do a toJS() on each array that arrive to React.

@planttheidea
Copy link
Owner

@andreigabreanu indeed, actually my solution was to learn from (steal?) seamless-immutable's solution. They don't go so far as to prevent all non-POJOs from being cloned, they explicitly detect if it is a React element and if it is then treat it as already immutable (see here).

I have tested this locally and it's working as expected, so I'm gonna release it soon and you can tell me if it solves your problem (if it doesn't, I'll do a more rigorous testing when I have more time this week). I have also included some documentation to the README about recursive objects because the real root of the issue is that React elements are recursive objects and you cannot make a recursive object truly immutable (seamless-immutable has the same disclosure).

@planttheidea
Copy link
Owner

alright, released v2.5.1 with the treatment of React elements as immutable, closing for now. If you have any more issues with this let me know!

@andreigabreanu
Copy link
Author

andreigabreanu commented Jul 10, 2016

Tested as well with 2.5.1. It works indeed.
But the performance has degraded a lot if I let the array as immutable and pass it to React. Keeping thaw() seems to be OK (as in same with 2.5.0). Keep in mind we're talking about 4 items in the array only although I probably have quite a few rerenders but still ...

I think this makes sense and I wouldn't say it is a bug in the library because the data is basically acting the proper way, the fact that react does some stuff in the loop and overall this makes everything slow is more or less a React issue.
Maybe just warn in the docs about this case (in case you can reproduce or other people reproduce) and that's it?

Anyway, thanks for the very fast help ! Have a great day!

@planttheidea
Copy link
Owner

planttheidea commented Jul 10, 2016

Actually it may not be a React issue... even though it's letting the object through, the object is still getting hashed for equality comparison, and hashing that recursive object may be slowing things down significantly. I'll see if I can tune it up.

EDIT

Indeed the hashing of the ReactElement was causing a significant slowdown (when mapping 10 elements it was taking 87ms, whereas the native was taking less than 1ms). In version 2.5.2 I have been able to cut that significantly so that the time is much closer to the native (difference is negligible until u get into mapping over 1000+ React elements).

One extra step that you can take, though, is setting up a generic utility function that uses the native map applied to the CrioArray. Example:

const thawMap = (crioArray, fn) => {
  return Array.prototype.map.call(crioArray, fn);
});

In my quick testing, this performed at the same rate as the native map across all sizes.

Thanks @andreigabreanu!

@andreigabreanu
Copy link
Author

I confirm it works good now ! (without the generic fn).

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

2 participants