Refactor fill component to avoid relying on componentWillUpdate and use React Hooks instead#15541
Conversation
…se React Hooks instead
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
jorgefilipecosta,
noisysocks and
talldan
as code owners
Some more context at #3472 (comment) . I think the main idea is to force all fills to render again via I worry a bit that it might rely on very specific lifecycle timing (the before render behavior of |
|
@aduth do you know if there's a test covering this as it would help here. |
|
@youknowriad There was a test case added in the relevant commit, yes. In that case, we may be able to judge by this passing. |
|
Ok tests are failing which is good as it confirms the issue but the issue is I have no idea how to fix this right now. I'll probably rethink about it tomorrow. |
|
I give up on this one, I was not able to make the tests pass. I feel like I'm not far from finding a solution though. |
|
I think I found a fix for the reordering issue, let's see what the tests say. |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
It's complex stuff. I don't fully understand all changes applied but based on Travis reaction I'm happy with the proposal :)
I strongly recommend another pair of eyes on this PR before you proceed.
| function FillComponent( { name, getSlot, children, registerFill, unregisterFill } ) { | ||
| // Random state used to rerender the component if needed, ideally we don't need this | ||
| const [ , updateRerenderState ] = useState( {} ); | ||
| const rerender = () => updateRerenderState( {} ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
yes, not sure which one is best. I guess it doesn't matter that much
|
@youknowriad just checking whether it was ever merged to master as I don't find it in the master that uses react-hooks. |
|
Yes, this was merged. you're maybe looking at the wrong master (your fork or something?) |
|
Oh sorry, i see it now. |
|
This RR might introduce a regression which isn't covered with tests: |
Per #15541 (comment), it is intended to be covered with tests, though if it is in-fact a regression, it would appear coverage could be improved. |
4 participants
Refs #11360
I'm not familiar with the details of the slot/fill implementation especially the occurences thing so please make sure to test this extensively.
Testing instructions