dustingetz / react-cursor

Immutable state for React.js
1.03k stars 50 forks source link

Cursors broken for functions. #19

Closed tolmasky closed 9 years ago

tolmasky commented 10 years ago

Took me hours to figure out, but the following won't work:

// Over consecutive calls... mycursor.refine("whatever").set(new Function("console.log(" + i + ")")

From what I can see poking around the code, functions don't really seem to figure into the hashing system for the memorizer very well. The strange construction in the above code (new function) is just to show that even when there is not toString equivalence it is still failing. Over consecutive calls the functions will be log(1), log(2), log(3), etc. However, Cursor.build(this) still returns the original log(1) version. If I remove the memorizer factory, everything starts working again.

dustingetz commented 10 years ago

Good find. It would be possible to memoize functions by reference (same as React cmps are hashed), but only if the application cares to preserve reference equality for functions in a cursor.

Perhaps you can get away with only storing simple values in react state, with well defined equality semantics? You can still pass functions as props.

danielmiladinov commented 10 years ago

That would be my recommendation as well. I can look for the reference, and I remember hearing that React state should be used for data that can be serialized to local storage, as opposed to functions or objects with behavior.

On Mon, Oct 6, 2014 at 1:45 PM, Dustin Getz notifications@github.com wrote:

Good find. It would be possible to memoize functions by reference (same as React cmps are hashed), but only if the application cares to preserve reference equality for functions in a cursor.

Perhaps you can get away with only storing simple values in react state, with well defined equality semantics?

— Reply to this email directly or view it on GitHub https://github.com/dustingetz/react-cursor/issues/19#issuecomment-58057311 .

dustingetz commented 9 years ago

Closing this as we have no plans to support storing non-value types in cursors - will address with better documentation in #48