search

Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
549 stars 83 forks source link

Added wrapping object for every type-declared record used as a prop object with `[<ReactComponentAttribute>]` #606

Open lukaszkrzywizna opened 6 months ago

lukaszkrzywizna commented 6 months ago

It resolves #603

Zaid-Ajaj commented 5 months ago

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 😅

lukaszkrzywizna commented 5 months ago

Thank you @Zaid-Ajaj for the response!

React was re-rendering things because the record got a new reference but the data hasn't changed.

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.

Personally I feel like we shouldn't change anything here.

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.

we would need to revive the unit test suite to ensure we are not breaking anyone with these changes

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. 😅

Zaid-Ajaj commented 5 months ago

Do you mean the 'Tests' project? I can try to revive this. Do you know what should be done?

Yeah that's the one. mostly going for github actions instead of appveyor, updating deps etc.