From 262b9c72f5f40a2d2ac8e2252ccbf296ee05e2e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 6 Jun 2017 15:19:38 -0700 Subject: [PATCH] Don't hydrate any properties other than event listeners and text content (#9858) * Don't hydrate any properties other than event listeners and text content This strategy assumes that the rendered HTML is correct if the tree lines up. Therefore we don't diff any attributes of the rendered HTML. However, as a precaution I ensure that textContent *is* updated. This ensures that if something goes wrong with keys lining up etc. at least there is some feedback that the event handlers might not line up. With what you expect. This might not be what you want e.g. for date formatting where it can different between server and client. It is expected that content will line up. To ensure that I will in a follow up ensure that the warning is issued if it doesn't line up so that in development this can be addressed. The text content updates are now moved to the commit phase so if the tree is asynchronously hydrated it doesn't start partially swapping out. I use the regular update side-effect with payload if the content doesn't match up. Since we no longer guarantee that attributes is correct I changed the bad mark up SSR integration tests to only assert on the textContent instead. * Hydrate text node if possible Currently we're never matching text nodes so we need to properly branch. --- scripts/fiber/tests-failing.txt | 6 +- scripts/fiber/tests-passing.txt | 4 + src/renderers/dom/fiber/ReactDOMFiber.js | 9 +- .../dom/fiber/ReactDOMFiberComponent.js | 138 +++++++++++++++++- .../dom/fiber/wrappers/ReactDOMFiberInput.js | 2 +- .../dom/fiber/wrappers/ReactDOMFiberOption.js | 2 +- .../dom/fiber/wrappers/ReactDOMFiberSelect.js | 6 +- .../fiber/wrappers/ReactDOMFiberTextarea.js | 2 +- .../ReactDOMServerIntegration-test.js | 39 ++++- .../shared/fiber/ReactFiberBeginWork.js | 2 +- .../shared/fiber/ReactFiberCommitWork.js | 18 ++- .../shared/fiber/ReactFiberCompleteWork.js | 35 +++-- .../fiber/ReactFiberHydrationContext.js | 78 ++++++---- .../shared/fiber/ReactFiberReconciler.js | 5 +- .../shared/fiber/ReactFiberScheduler.js | 8 +- 15 files changed, 278 insertions(+), 76 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 789af77694..8b13789179 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -1,5 +1 @@ -src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js -* should not blow away user-entered text on successful reconnect to an uncontrolled checkbox -* should not blow away user-entered text on successful reconnect to a controlled checkbox -* should not blow away user-selected value on successful reconnect to an uncontrolled select -* should not blow away user-selected value on successful reconnect to an controlled select + diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 930fb89124..4f724b760d 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1226,6 +1226,10 @@ src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * renders a controlled select with client render on top of good server markup * should not blow away user-entered text on successful reconnect to an uncontrolled input * should not blow away user-entered text on successful reconnect to a controlled input +* should not blow away user-entered text on successful reconnect to an uncontrolled checkbox +* should not blow away user-entered text on successful reconnect to a controlled checkbox +* should not blow away user-selected value on successful reconnect to an uncontrolled select +* should not blow away user-selected value on successful reconnect to an controlled select * renders class child with context with server string render * renders class child with context with clean client render * renders class child with context with client render on top of good server markup diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index b17a2d40f6..31cafd4873 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -48,6 +48,7 @@ var { setInitialProperties, diffProperties, updateProperties, + diffHydratedProperties, } = ReactDOMFiberComponent; var {precacheFiberNode, updateFiberProps} = ReactDOMComponentTree; @@ -409,19 +410,21 @@ var DOMRenderer = ReactFiberReconciler({ props: Props, rootContainerInstance: Container, internalInstanceHandle: Object, - ): void { + ): null | Array { precacheFiberNode(internalInstanceHandle, instance); // TODO: Possibly defer this until the commit phase where all the events // get attached. updateFiberProps(instance, props); - setInitialProperties(instance, type, props, rootContainerInstance); + return diffHydratedProperties(instance, type, props, rootContainerInstance); }, hydrateTextInstance( textInstance: TextInstance, + text: string, internalInstanceHandle: Object, - ): void { + ): boolean { precacheFiberNode(internalInstanceHandle, textInstance); + return textInstance.nodeValue !== text; }, scheduleAnimationCallback: ReactDOMFrameScheduling.rAF, diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 5c29bd810f..2406de28db 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -177,10 +177,10 @@ function setInitialDOMProperties( isCustomComponentTag: boolean, ): void { for (var propKey in nextProps) { - var nextProp = nextProps[propKey]; if (!nextProps.hasOwnProperty(propKey)) { continue; } + var nextProp = nextProps[propKey]; if (propKey === STYLE) { if (__DEV__) { if (nextProp) { @@ -406,7 +406,7 @@ var ReactDOMFiberComponent = { props = rawProps; break; case 'input': - ReactDOMFiberInput.mountWrapper(domElement, rawProps); + ReactDOMFiberInput.initWrapperState(domElement, rawProps); props = ReactDOMFiberInput.getHostProps(domElement, rawProps); trapBubbledEventsLocal(domElement, tag); // For controlled components we always need to ensure we're listening @@ -414,11 +414,11 @@ var ReactDOMFiberComponent = { ensureListeningTo(rootContainerElement, 'onChange'); break; case 'option': - ReactDOMFiberOption.mountWrapper(domElement, rawProps); + ReactDOMFiberOption.validateProps(domElement, rawProps); props = ReactDOMFiberOption.getHostProps(domElement, rawProps); break; case 'select': - ReactDOMFiberSelect.mountWrapper(domElement, rawProps); + ReactDOMFiberSelect.initWrapperState(domElement, rawProps); props = ReactDOMFiberSelect.getHostProps(domElement, rawProps); trapBubbledEventsLocal(domElement, tag); // For controlled components we always need to ensure we're listening @@ -426,7 +426,7 @@ var ReactDOMFiberComponent = { ensureListeningTo(rootContainerElement, 'onChange'); break; case 'textarea': - ReactDOMFiberTextarea.mountWrapper(domElement, rawProps); + ReactDOMFiberTextarea.initWrapperState(domElement, rawProps); props = ReactDOMFiberTextarea.getHostProps(domElement, rawProps); trapBubbledEventsLocal(domElement, tag); // For controlled components we always need to ensure we're listening @@ -462,6 +462,9 @@ var ReactDOMFiberComponent = { case 'option': ReactDOMFiberOption.postMountWrapper(domElement, rawProps); break; + case 'select': + ReactDOMFiberSelect.postMountWrapper(domElement, rawProps); + break; default: if (typeof props.onClick === 'function') { // TODO: This cast may not be sound for SVG, MathML or custom elements. @@ -704,6 +707,131 @@ var ReactDOMFiberComponent = { } }, + diffHydratedProperties( + domElement: Element, + tag: string, + rawProps: Object, + rootContainerElement: Element | Document, + ): null | Array { + if (__DEV__) { + var isCustomComponentTag = isCustomComponent(tag, rawProps); + validatePropertiesInDevelopment(tag, rawProps); + if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) { + warning( + false, + '%s is using shady DOM. Using shady DOM with React can ' + + 'cause things to break subtly.', + getCurrentFiberOwnerName() || 'A component', + ); + didWarnShadyDOM = true; + } + } + + switch (tag) { + case 'audio': + case 'form': + case 'iframe': + case 'img': + case 'image': + case 'link': + case 'object': + case 'source': + case 'video': + case 'details': + trapBubbledEventsLocal(domElement, tag); + break; + case 'input': + ReactDOMFiberInput.initWrapperState(domElement, rawProps); + trapBubbledEventsLocal(domElement, tag); + // For controlled components we always need to ensure we're listening + // to onChange. Even if there is no listener. + ensureListeningTo(rootContainerElement, 'onChange'); + break; + case 'option': + ReactDOMFiberOption.validateProps(domElement, rawProps); + break; + case 'select': + ReactDOMFiberSelect.initWrapperState(domElement, rawProps); + trapBubbledEventsLocal(domElement, tag); + // For controlled components we always need to ensure we're listening + // to onChange. Even if there is no listener. + ensureListeningTo(rootContainerElement, 'onChange'); + break; + case 'textarea': + ReactDOMFiberTextarea.initWrapperState(domElement, rawProps); + trapBubbledEventsLocal(domElement, tag); + // For controlled components we always need to ensure we're listening + // to onChange. Even if there is no listener. + ensureListeningTo(rootContainerElement, 'onChange'); + break; + } + + assertValidProps(tag, rawProps, getCurrentFiberOwnerName); + + var updatePayload = null; + for (var propKey in rawProps) { + if (!rawProps.hasOwnProperty(propKey)) { + continue; + } + var nextProp = rawProps[propKey]; + if (propKey === CHILDREN) { + // For text content children we compare against textContent. This + // might match additional HTML that is hidden when we read it using + // textContent. E.g. "foo" will match "foo" but that still + // satisfies our requirement. Our requirement is not to produce perfect + // HTML and attributes. Ideally we should preserve structure but it's + // ok not to if the visible content is still enough to indicate what + // even listeners these nodes might be wired up to. + // TODO: Warn if there is more than a single textNode as a child. + // TODO: Should we use domElement.firstChild.nodeValue to compare? + if (typeof nextProp === 'string') { + if (domElement.textContent !== nextProp) { + updatePayload = [CHILDREN, nextProp]; + } + } else if (typeof nextProp === 'number') { + if (domElement.textContent !== '' + nextProp) { + updatePayload = [CHILDREN, '' + nextProp]; + } + } + } else if (registrationNameModules.hasOwnProperty(propKey)) { + if (nextProp) { + ensureListeningTo(rootContainerElement, propKey); + } + } + } + + switch (tag) { + case 'input': + // TODO: Make sure we check if this is still unmounted or do any clean + // up necessary since we never stop tracking anymore. + inputValueTracking.trackNode((domElement: any)); + ReactDOMFiberInput.postMountWrapper(domElement, rawProps); + break; + case 'textarea': + // TODO: Make sure we check if this is still unmounted or do any clean + // up necessary since we never stop tracking anymore. + inputValueTracking.trackNode((domElement: any)); + ReactDOMFiberTextarea.postMountWrapper(domElement, rawProps); + break; + case 'select': + case 'option': + // For input and textarea we current always set the value property at + // post mount to force it to diverge from attributes. However, for + // option and select we don't quite do the same thing and select + // is not resilient to the DOM state changing so we don't do that here. + // TODO: Consider not doing this for input and textarea. + break; + default: + if (typeof rawProps.onClick === 'function') { + // TODO: This cast may not be sound for SVG, MathML or custom elements. + trapClickOnNonInteractiveElement(((domElement: any): HTMLElement)); + } + break; + } + + return updatePayload; + }, + restoreControlledState( domElement: Element, tag: string, diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index 17b0f3df02..df430502db 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js @@ -89,7 +89,7 @@ var ReactDOMInput = { return hostProps; }, - mountWrapper: function(element: Element, props: Object) { + initWrapperState: function(element: Element, props: Object) { if (__DEV__) { ReactControlledValuePropTypes.checkPropTypes( 'input', diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js index 7f0a25935a..781f94924d 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js @@ -39,7 +39,7 @@ function flattenChildren(children) { * Implements an