Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null reference scenario with unkeyed children #753

Closed
mstijak opened this issue Jul 11, 2017 · 7 comments · Fixed by #1440
Closed

Null reference scenario with unkeyed children #753

mstijak opened this issue Jul 11, 2017 · 7 comments · Fixed by #1440
Assignees

Comments

@mstijak
Copy link

mstijak commented Jul 11, 2017

I found a problem with using ref on unkeyed children. You can observe the problem here: https://jsfiddle.net/rsm2dxb7/1/
Open the console and click Toggle. You'll see that the final dom reference being passed is null, which is wrong because the referenced div is still there.

In the example, the first child is conditionally rendered and the second child is using a ref. On click, the state changes and the first child is unmounted. The ref callback is called two times in wrong order, so the final reference value is null.

{ header && <div className="header">Header</div> }
<div ref={c=>{
  console.log(c);
}}>
  Content
</div>
@mstijak
Copy link
Author

mstijak commented Jul 11, 2017

Here's a failing test:

it('should correctly call child refs for un-keyed children on re-render', () => {
	let el = null;
	let ref = e => { el = e };

	class App extends Component {
		render({ headerVisible }) {
			return <div>
				{ headerVisible && <div>Header</div> }
				<div className="content" ref={ref}>
					Content
				</div>
			</div>;
		}
	}

	let root = render(<App headerVisible />, scratch);
	expect(el).to.not.be.equal(null);
	root = render(<App />, scratch, root);
	expect(el).to.not.be.equal(null);
});

@developit
Copy link
Member

Interesting, this seems to happen due to elements shifting on removal.
It seems like the removed element in this case should be bypassed in the next diff pass:
https://github.com/developit/preact/blob/master/src/vdom/diff.js#L228

@mstijak
Copy link
Author

mstijak commented Jul 13, 2017

I read somewhere that preact v8 preserves empty slots. That approach might also be useful here, as in that case, the second div would not morph into the first one.

@developit
Copy link
Member

Yup - that's why I was surprised to see this issue.

@Rendalf
Copy link

Rendalf commented Mar 16, 2018

Recently stumbled across the same issue.
Any options how to fix/avoid that?

@yaodingyd
Copy link
Collaborator

Essentially duplicate of #857.
Although this was raised earlier, looks like the main discussion is in #857.
@Rendalf For now unkeyed diff has this issue which cannot be resolved. You can use keyed children if possible.

@marvinhagemeister
Copy link
Member

Related to #1440

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants