-
Notifications
You must be signed in to change notification settings - Fork 51k
Don't hydrate any properties other than event listeners and text content #9858
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,27 +406,27 @@ 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 | ||
| // to onChange. Even if there is no listener. | ||
| 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 | ||
| // to onChange. Even if there is no listener. | ||
| 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<mixed> { | ||
| 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 "f<span>oo</span>" 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of For example, for the JSX You are deliberately not checking everything in the tree, though, and the documentation for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is interesting though. Since the goal here isn't to preserve the correct document structure and attributes etc, but actually only to preserve text content in terms of how they might line up to critical event handlers this still satisfies that requirement. Even if the text content doesn't fully line up. I'll add a comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. One other thought I just had: what about when children is an array of text or an array of numbers (or an array of arrays of strings, etc.)? I think this code will fail to fix up the text in that case, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will go through another pass in Fiber that deals with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And hence my comment about still not understanding the Fiber internals. Thanks for your patience; I'll be quiet now. 😁 |
||
| 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, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,11 +106,31 @@ const clientRenderOnServerString = async (element, errorCount = 0) => { | |
| return clientElement; | ||
| }; | ||
|
|
||
| const clientRenderOnBadMarkup = (element, errorCount = 0) => { | ||
| function BadMarkupExpected() {} | ||
|
|
||
| const clientRenderOnBadMarkup = async (element, errorCount = 0) => { | ||
| // First we render the top of bad mark up. | ||
| var domElement = document.createElement('div'); | ||
| domElement.innerHTML = | ||
| '<div id="badIdWhichWillCauseMismatch" data-reactroot="" data-reactid="1"></div>'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd argue that it's probably a good idea to put in some junk text here to make sure that the patch up occurs. It's not impossible to assume that some tests try rendering
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is already other tests in other places in the test suite that covers this so I think it's mostly fine. What we don't have is a test that ensures that the text node is preserved (which I missed in the original PR). However, I think that's more of an optimization than correctness.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought at one point about having the test in the hydrating "good markup" case actually walk the entire DOM tree before and after rendering to confirm that the nodes were not replaced by hydration. Would that be helpful as a PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't want to be too prescriptive but if it is a completely good tree that's probably pretty inherent to the whole model. So, yes, that would be great.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out I can't put junk text in there because |
||
| return renderIntoDom(element, domElement, errorCount + 1); | ||
| await renderIntoDom(element, domElement, errorCount + 1); | ||
|
|
||
| // This gives us the resulting text content. | ||
| var hydratedTextContent = domElement.textContent; | ||
|
|
||
| // Next we render the element into a clean DOM node client side. | ||
| const cleanDomElement = document.createElement('div'); | ||
| ExecutionEnvironment.canUseDOM = true; | ||
| await asyncReactDOMRender(element, cleanDomElement); | ||
| ExecutionEnvironment.canUseDOM = false; | ||
| // This gives us the expected text content. | ||
| const cleanTextContent = cleanDomElement.textContent; | ||
|
|
||
| // The only guarantee is that text content has been patched up if needed. | ||
| expect(hydratedTextContent).toBe(cleanTextContent); | ||
|
|
||
| // Abort any further expects. All bets are off at this point. | ||
| throw new BadMarkupExpected(); | ||
| }; | ||
|
|
||
| // runs a DOM rendering test as four different tests, with four different rendering | ||
|
|
@@ -148,8 +168,17 @@ function itClientRenders(desc, testFn) { | |
| testFn(clientCleanRender)); | ||
| it(`renders ${desc} with client render on top of good server markup`, () => | ||
| testFn(clientRenderOnServerString)); | ||
| it(`renders ${desc} with client render on top of bad server markup`, () => | ||
| testFn(clientRenderOnBadMarkup)); | ||
| it(`renders ${desc} with client render on top of bad server markup`, async () => { | ||
| try { | ||
| await testFn(clientRenderOnBadMarkup); | ||
| } catch (x) { | ||
| // We expect this to trigger the BadMarkupExpected rejection. | ||
| if (!(x instanceof BadMarkupExpected)) { | ||
| // If not, rethrow. | ||
| throw x; | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function itThrows(desc, testFn) { | ||
|
|
@@ -425,7 +454,7 @@ describe('ReactDOMServerIntegration', () => { | |
|
|
||
| itRenders('no dangerouslySetInnerHTML attribute', async render => { | ||
| const e = await render( | ||
| <div dangerouslySetInnerHTML={{__html: 'foo'}} />, | ||
| <div dangerouslySetInnerHTML={{__html: '<foo />'}} />, | ||
| ); | ||
| expect(e.getAttribute('dangerouslySetInnerHTML')).toBe(null); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉