fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
273 stars 67 forks source link

Suggestion: Rename/clarify memoEqualsButFunctions #170

Closed penguid closed 5 years ago

penguid commented 5 years ago

I have a slight problem with the memoEqualsButFunctions function. I think it should be made more clear that it has nothing to do with any F# equality or F# value/reference distinctions, but is instead entirely dependent on the JavaScript === operator in conjunction with the internal JavaScript-translated type implementations.

Thus, it somewhat unintuitively (unless you understand the internals) does things like value-compare options that wrap a primitive (because the option, ostensibly a reference type, is erased), and reference-compare dates and struct records (because they are not value types in JS).

I do understand that devs need to always have these .NET/JS compatibility issues in mind when developing in Fable, and I'm not arguing that any of the above behavior is wrong. I just feel that it makes this particular function much more of a trap than usual. It has a somewhat "blessed" name with the memo- prefix that might imply it's an easy default "pit of success" choice to use to memoize your view component, when in reality you should be very clear on exactly what you're getting yourself into (ensuring stable references, no on-the-fly DUs/anon records, etc.) before choosing this over standard structural equality.

Maybe it would be better named something like jsIdentityEquals instead of memoEquals. Also maybe the doc comment could use the term primitives instead of value types to avoid any false expectations regarding struct types.

Or maybe this is an overreaction to a non-issue, I don't know.

MangelMaxime commented 5 years ago

Hum, we do have an helpers which according to the documentation works as an F# equality, see equalsButFunctions

About memoEqualsButFunctions, the documentation is mentioning that it's doing value comparison for value types and strings and reference comparison other types.

Perhaps, adding what other types means is enough to avoid the confusion.

penguid commented 5 years ago

Closing as user education issue, no need to bikeshed or distract from other issues.

alfonsogarciacaro commented 4 years ago

Yes, you're right @penguid that the name is not very informative. Unfortunately other alternatives are not perfect either as they miss information too (unless we use a very, very long name). We first added equalsButFunctions that emulates F# structural equality but ignores functions (because users usually pass new references to the dispatch function, etc, which break equality). Then we added memoEqualsButFunctions as a way to emulate the default equality in React.memo (structural equality but only at the the top level and then JS identity) but also ignoring functions. We did this because because the latter should be faster than the former (equalsButFunctions) but still valid for most cases.

reinux commented 1 year ago

I'm actually confused about this myself, though I'm not sure whether it's the kind of confusion that this issue is about.

If for the props I have a tuple of arguments, with one item being the Elmish dispatch, is it supposed to compare everything else except the dispatch? Because for me it seems to return false if the dispatch is in there.

MangelMaxime commented 1 year ago

@reinux React props should be POJO which are records or anonymous records in F#.

After that, if you have one of the property of the records which is a tuple value I think the tuple is recreated on each rendering leading to an equality which always returns false.

If needed, you can provide your own equality function for that specific component if needed.

reinux commented 1 year ago

Ah, that explains it. There's a DU that I'm recreating each time, which is causing it to be invalidated. And I'm assuming JavaScript doesn't distinguish between struct and reference types for comparison, so a custom equality function is probably the way to go.

Thanks!