RFC: Should we be resilient to node.normalize()?

[sebmarkbage]

Debate: Should we support the node.normalize() case?

react/src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js

Lines 77 to 173 in b1b4a2f

	it('can reconcile text merged by Node.normalize() alongside other elements', () => {
	var el = document.createElement('div');
	var inst = ReactDOM.render(
	<div>{'foo'}{'bar'}{'baz'}<span />{'qux'}</div>,
	el,
	);
	var container = ReactDOM.findDOMNode(inst);
	container.normalize();
	inst = ReactDOM.render(<div>{'bar'}{'baz'}{'qux'}<span />{'foo'}</div>, el);
	container = ReactDOM.findDOMNode(inst);
	expect(container.textContent).toBe('barbazquxfoo');
	});
	it('can reconcile text merged by Node.normalize()', () => {
	var el = document.createElement('div');
	var inst = ReactDOM.render(<div>{'foo'}{'bar'}{'baz'}</div>, el);
	var container = ReactDOM.findDOMNode(inst);
	container.normalize();
	inst = ReactDOM.render(<div>{'bar'}{'baz'}{'qux'}</div>, el);
	container = ReactDOM.findDOMNode(inst);
	expect(container.textContent).toBe('barbazqux');
	});
	it('can reconcile text from pre-rendered markup', () => {
	var el = document.createElement('div');
	var reactEl = <div>{'foo'}{'bar'}{'baz'}</div>;
	el.innerHTML = ReactDOMServer.renderToString(reactEl);
	ReactDOM.render(reactEl, el);
	expect(el.textContent).toBe('foobarbaz');
	reactEl = <div>{''}{''}{''}</div>;
	el.innerHTML = ReactDOMServer.renderToString(reactEl);
	ReactDOM.render(reactEl, el);
	expect(el.textContent).toBe('');
	});
	it('can reconcile text arbitrarily split into multiple nodes', () => {
	var el = document.createElement('div');
	var inst = ReactDOM.render(<div><span />{'foobarbaz'}</div>, el);
	var container = ReactDOM.findDOMNode(inst);
	let childNodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
	let textNode = childNodes[1];
	textNode.textContent = 'foo';
	container.insertBefore(
	document.createTextNode('bar'),
	childNodes[1].nextSibling,
	);
	container.insertBefore(
	document.createTextNode('baz'),
	childNodes[1].nextSibling,
	);
	inst = ReactDOM.render(<div><span />{'barbazqux'}</div>, el);
	container = ReactDOM.findDOMNode(inst);
	expect(container.textContent).toBe('barbazqux');
	});
	it('can reconcile text arbitrarily split into multiple nodes on some substitutions only', () => {
	var el = document.createElement('div');
	var inst = ReactDOM.render(
	<div><span />{'bar'}<span />{'foobarbaz'}{'foo'}{'barfoo'}<span /></div>,
	el,
	);
	var container = ReactDOM.findDOMNode(inst);
	let childNodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
	let textNode = childNodes[3];
	textNode.textContent = 'foo';
	container.insertBefore(
	document.createTextNode('bar'),
	childNodes[3].nextSibling,
	);
	container.insertBefore(
	document.createTextNode('baz'),
	childNodes[3].nextSibling,
	);
	let secondTextNode = childNodes[5];
	secondTextNode.textContent = 'bar';
	container.insertBefore(
	document.createTextNode('foo'),
	childNodes[5].nextSibling,
	);
	inst = ReactDOM.render(
	<div><span />{'baz'}<span />{'barbazqux'}{'bar'}{'bazbar'}<span /></div>,
	el,
	);
	container = ReactDOM.findDOMNode(inst);
	expect(container.textContent).toBe('bazbarbazquxbarbazbar');
	});

Summary: We mostly supported this in Stack because we inserted comment nodes between everything. This was important when we used innerHTML to generate the HTML on the client to preserve everything but it's not important anymore. The new hydration is resilient to this. However, if this happens on an already active client tree we're not. It's possible for consecutive text nodes to be merged into a single one if something above calls node.normalize(). The whole subtree merges consecutive text nodes. It's possible that extensions might do even more magic. https://developer.mozilla.org/en-US/docs/Web/API/Node/normalize

Context: I think I have an idea for how we can do an automatic recovery phase for this that wouldn't impact hot paths so it wouldn't normally need to do anything. However, it would add a small but non-trivial amount of code for an edge case that almost never happens. It is also a bit awkward since it adds a very DOM specific case to the HostConfig API that every renderer will have to consider.

AFAIK, we have not had any bugs related to this on FB with regard to extensions or anything else. How do we find out if this breaks things outside of FB without just shipping a broken version?

[sebmarkbage]

The resolution is that we'll ship a version to see how badly this breaks and under which conditions to get more data.

[gaearon]

¯\(ツ)/¯

[jinglesthula]

For what it's worth:

This and an entire related class of problems come down to one underlying thing: the DOM is shared mutable state.

The browser extension model makes the deal with the user that they can do anything they want to the DOM, but the implied caveat (which users won't likely be aware of) is that there is risk involved.

Consider the case of a jQuery driven site that relies on a particular DOM structure, classes, ids, etc. I can load an extension or a user script that violates the jQuery code's assumptions and "break the site". What's happening here with

if something above calls node.normalize()

is no different. If you create a web site or web application and other javascripts enter like so many deranged monkeys, all bets are off.

It would be lovely if browsers offered a mechanism for scripts to play nicely with each other, but unless that happens, code which relies on the DOM to behave in a certain way are subject to the forces of chaos.