Added wrapping object for every type-declared record used as a prop object with [<ReactComponentAttribute>]
#606
Conversation
…object with `[<ReactComponentAttribute>]`
lukaszkrzywizna
force-pushed
the
react-component-record-wrapping
branch
from
8056457 to
bacf81d
Compare
| // JS createElement(Component, { props = { Value: 1 } }) | ||
|
|
||
| // anonymous record | ||
| // F# Component { Value = 1 } |
There was a problem hiding this comment.
For clarity we could perhaps use the anonymous syntax ?
// F# Component {| Value = 1 |}
|
Hi @lukaszkrzywizna I know it's been a while since we implemented the record wrapping in its current state. I can't seem to remember why exactly it was needed 🤔 maybe for cases where React was re-rendering things because the record got a new reference but the data hasn't changed. Personally I feel like we shouldn't change anything here. It's hard to maintain F#-ness of things when going into React world. Keep things simple and when you need F#-ness maintained, use anonymous record to wrap things. Also before adding something like this, we would need to revive the unit test suite to ensure we are not breaking anyone with these changes 😅 |
- Added support for records with a lowercased 'key' property. - Added warning for anonymous records used as props-object with a lowercased 'key' property.
|
Thank you @Zaid-Ajaj for the response!
Hmm... I don't recognize any mechanism similar to this. Maybe React.memo? But according to documentation it compares props one-by-one, not the whole props object.
I would agree, but tracking down the issue in my production app cost me many hours. The record passed as an argument was used in a dispatch message where the handler retrieved it and checked if the map contained it. The entire record prototype was erased, so it couldn't work properly, causing the problem to manifest far away from the actual cause in the app. Any information from the compiler would be a huge help in the future, but I understand that changing the behavior of record construction could be significant. That's why I propose at least showing a warning.
Do you mean the 'Tests' project? I can try to revive this. Do you know what should be done? Besides that, I have been using a forked version with the fix in production for two weeks, and so far, no errors. 😅 |
Yeah that's the one. mostly going for github actions instead of appveyor, updating deps etc. |
3 participants
It resolves #603