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. |
| 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.
According to @gaearon
https://twitter.com/dan_abramov/status/1118301474876948480
you can do this:
const [, forceUpdate] = useReducer(x => x + 1, 0)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. |
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