Open syranide opened 8 years ago
Also came here to report this. Prevents a very usual usecase for focus
/select
:
<input
autoFocus
type="text"
ref="valueInput"
onFocus={this.onFocus}
/>
// this.onFocus:
this.refs.valueInput.select();
^ refs are not available yet
Using a workaround now:
// this.onFocus:
ReactDOM.findDOMNode(this).children[0]
@LeZuse You can just use event.target
as a simpler workaround FYI.
@syranide ha, true! Thanks!
Still happens on 16.
@syranide The event.target
workaround works great. For everybody else, you might need event.currentTarget
, be aware of the difference, and pick the one that fit your needs.
Not sure what the right fix would be. Tagging as good first issue for somebody to investigate.
The issue here seems to be that Ref
effects are called after Update
effects below (the latter being the effect to do the mounting). Although, if this is the issue, I'm not sure why it would work in IE11 🤔 ...
I'm assuming this is to ensure that all refs are guaranteed to be mounted, which seems to make sense. On the off-chance that this isn't a guarantee that React provides then we could just flip the order of effects processing (although this breaks two tests around refs being called at the correct time, suggesting this is a hard constraint). Otherwise, it seems autofocussing would need to be broken away from the mounting process to solve this issue, which seems like an odd thing to do for other reasons.
Edit:
To be clear: the update gets marked here if finalizeInitialChildren
returns true (which it does only if the host component needs an autofocus).
That then gets fired on commitMount
in the reconciler, which again, makes sense.
Nobody else has picked up this issue? I'd love to give it a try if not
I had a look at this but I feel like it would mean a relatively big change and could probably do with a bit of feedback from @gaearon before pushing on.
Yeah good point, it looks like this would require changing the order in which different effects are applied to elements when they are created/updated, and changing that would obviously have a lot of side-effects.
I'll leave this be and find something else to work on for now
Exactly, unless I’ve misunderstood something. If you can find another way I’d be interested to hear!
On Sun, 4 Feb 2018 at 13:16, Portfolio-rails notifications@github.com wrote:
Yeah good point, it looks like this would require changing the order in which different effects are applied to elements when they are created/updated, and changing that would obviously have a lot of side-effects.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/7769#issuecomment-362905922, or mute the thread https://github.com/notifications/unsubscribe-auth/ABk124JjxK9pYO-wgsbz2iYTec8L-1NHks5tRa29gaJpZM4KA9JI .
hey guys, I'd like to take up this issue if its available
Another broad idea to fix this (a few years on) is to swap the order of Ref
and Update
but delay hooks until the Ref
effects have run. No idea how feasible this would be.
Encountered this issue when using useImperativeHandle
. The issue is that refs can be set with useImperativeHandle
after useLayoutEffects
run. This can become an issue if we execute callbacks in useLayoutEffect
that use the ref set by useImperativeHandle
e.g.
https://codesandbox.io/s/dank-darkness-podtr
const Child = React.forwardRef(({ callback }, ref) => {
React.useLayoutEffect(callback);
React.useImperativeHandle(ref, () => () =>
console.log("handled me imperatively")
);
return null;
});
function Parent() {
const childRef = React.useRef(null);
// crash unless order of effects in Child is swapped
return <Child callback={() => childRef.current()} ref={childRef} />;
}
Tried @RichieAHB proposed fix which resulted in 2 test failures which are desired IMO:
diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js
index d3c5cbc1f..450f053e5 100644
--- a/packages/react-dom/src/__tests__/ReactComponent-test.js
+++ b/packages/react-dom/src/__tests__/ReactComponent-test.js
@@ -356,10 +356,10 @@ describe('ReactComponent', () => {
'start mount',
'inner 1 render',
'inner 2 render',
- 'inner 1 componentDidMount',
'ref 1 got instance 1',
- 'inner 2 componentDidMount',
+ 'inner 1 componentDidMount',
'ref 2 got instance 2',
+ 'inner 2 componentDidMount',
'outer componentDidMount',
'start update',
// Previous (equivalent) refs get cleared
@@ -368,10 +368,10 @@ describe('ReactComponent', () => {
'inner 2 render',
'ref 1 got null',
'ref 2 got null',
- 'inner 1 componentDidUpdate',
'ref 1 got instance 1',
- 'inner 2 componentDidUpdate',
+ 'inner 1 componentDidUpdate',
'ref 2 got instance 2',
+ 'inner 2 componentDidUpdate',
'outer componentDidUpdate',
'start unmount',
'outer componentWillUnmount',
diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
index fd286f1e6..355112e0b 100644
--- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
+++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
@@ -1046,12 +1046,12 @@ describe('ReactComponentLifeCycle', () => {
expect(log).toEqual([
'will mount',
'render',
- 'did mount',
'ref',
+ 'did mount',
'render',
- 'did update',
'ref',
+ 'did update',
]);
});
Basically: Refs are called before didUpdate/didMount which should make more sense. If the component already did mount and did update then I should already have the updated refs and not was is currently after-didUpdate.
The issue becomes more apparent with forwardRef
. If I pass forwarded refs to host components then those are set before layout effects run. But if I manually handle them with useImperativeHandle
and not carefully check that I place this at the top of my render function I get a slightly different behavior.
Is this something the core team would be interested in or should this be re-opened when a breaking change is scheduled?
https://jsfiddle.net/nnwd2c34/
Can reproduce on Chrome, but not in IE11.