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.

Sign up for GitHub

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

Jump to bottom

Fix false positive hydration warning for SVG attributes #10676

Merged
merged 3 commits into from Sep 13, 2017
Merged

Fix false positive hydration warning for SVG attributes #10676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e900de
Fix false positive hydration warning for SVG attributes
gaearon Sep 11, 2017
ce09e18
Undo the generic fix
gaearon Sep 12, 2017
8c30d5b
Pass namespace through and use it to determine sensitivity
gaearon Sep 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Expand Up @@ -776,6 +776,7 @@ var ReactDOMFiberComponent = {
domElement: Element,
tag: string,
rawProps: Object,
parentNamespace: string,
rootContainerElement: Element | Document,
): null | Array<mixed> {
if (__DEV__) {
Expand Down Expand Up @@ -912,6 +913,8 @@ var ReactDOMFiberComponent = {
case 'selected':
break;
default:
// Intentionally use the original name.
// See discussion in https://github.com/facebook/react/pull/10676.
extraAttributeNames.add(attributes[i].name);
}
}
Expand Down Expand Up @@ -1007,8 +1010,17 @@ var ReactDOMFiberComponent = {
nextProp,
);
} else {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey.toLowerCase());
let ownNamespace = parentNamespace;
if (ownNamespace === HTML_NAMESPACE) {
Copy link
Contributor

@nhunzaker nhunzaker Sep 12, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rename parentNamespace to ownNamespace here instead of using parentNamespace directly or setting ownNamespace as the argument?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard. Sorry I didn't catch the code below.

ownNamespace = getIntrinsicNamespace(tag);
}
if (ownNamespace === HTML_NAMESPACE) {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth adding a comment for why we need to downcase the propKey (because HTML reports lowercase, right?)

} else {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me realize that we probably don't validate that two components with the same name but different namespaces line up in the hydration. I'm not sure there's a way to get that wrong in another way than innerHTML SVG content in a HTML root.

serverValue = DOMPropertyOperations.getValueForAttribute(
domElement,
propKey,
Expand Down
16 changes: 15 additions & 1 deletion src/renderers/dom/fiber/ReactDOMFiberEntry.js
View file
Open in desktop
Expand Up @@ -488,13 +488,27 @@ var DOMRenderer = ReactFiberReconciler({
type: string,
props: Props,
rootContainerInstance: Container,
hostContext: HostContext,
internalInstanceHandle: Object,
): null | Array<mixed> {
precacheFiberNode(internalInstanceHandle, instance);
// TODO: Possibly defer this until the commit phase where all the events
// get attached.
updateFiberProps(instance, props);
return diffHydratedProperties(instance, type, props, rootContainerInstance);
let parentNamespace: string;
if (__DEV__) {
const hostContextDev = ((hostContext: any): HostContextDev);
parentNamespace = hostContextDev.namespace;
} else {
parentNamespace = ((hostContext: any): HostContextProd);
}
return diffHydratedProperties(
instance,
type,
props,
parentNamespace,
rootContainerInstance,
);
},

hydrateTextInstance(
Expand Down
29 changes: 20 additions & 9 deletions src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js
View file
Open in desktop
Expand Up @@ -1325,19 +1325,30 @@ describe('ReactDOMServerIntegration', () => {
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
});

itRenders('svg child element', async render => {
let e = await render(
<svg><image xlinkHref="http://i.imgur.com/w7GCRPb.png" /></svg>,
);
e = e.firstChild;
itRenders('svg child element with an attribute', async render => {
let e = await render(<svg viewBox="0 0 0 0" />);
expect(e.childNodes.length).toBe(0);
expect(e.tagName).toBe('image');
expect(e.tagName).toBe('svg');
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe(
'http://i.imgur.com/w7GCRPb.png',
);
expect(e.getAttribute('viewBox')).toBe('0 0 0 0');
});

itRenders(
'svg child element with a namespace attribute',
Copy link
Collaborator Author

@gaearon gaearon Sep 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an old test. I just renamed it and moved down.

async render => {
let e = await render(
<svg><image xlinkHref="http://i.imgur.com/w7GCRPb.png" /></svg>,
);
e = e.firstChild;
expect(e.childNodes.length).toBe(0);
expect(e.tagName).toBe('image');
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe(
'http://i.imgur.com/w7GCRPb.png',
);
},
);

itRenders('svg child element with a badly cased alias', async render => {
let e = await render(
<svg><image xlinkhref="http://i.imgur.com/w7GCRPb.png" /></svg>,
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Expand Up @@ -72,7 +72,7 @@ if (__DEV__) {
module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
hostContext: HostContext<C, CX>,
hydrationContext: HydrationContext<C>,
hydrationContext: HydrationContext<C, CX>,
scheduleUpdate: (fiber: Fiber, priorityLevel: PriorityLevel) => void,
getPriorityContext: (fiber: Fiber, forceAsync: boolean) => PriorityLevel,
) {
Expand Down
3 changes: 2 additions & 1 deletion src/renderers/shared/fiber/ReactFiberCompleteWork.js
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@ var invariant = require('fbjs/lib/invariant');
module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
hostContext: HostContext<C, CX>,
hydrationContext: HydrationContext<C>,
hydrationContext: HydrationContext<C, CX>,
) {
const {
createInstance,
Expand Down Expand Up @@ -285,6 +285,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
prepareToHydrateHostInstance(
workInProgress,
rootContainerInstance,
currentHostContext,
)
) {
// If changes to the hydrated node needs to be applied at the
Expand Down
12 changes: 9 additions & 3 deletions src/renderers/shared/fiber/ReactFiberHydrationContext.js
View file
Open in desktop
Expand Up @@ -22,18 +22,22 @@ const {Deletion, Placement} = require('ReactTypeOfSideEffect');

const {createFiberFromHostInstanceForDeletion} = require('ReactFiber');

export type HydrationContext<C> = {
export type HydrationContext<C, CX> = {
enterHydrationState(fiber: Fiber): boolean,
resetHydrationState(): void,
tryToClaimNextHydratableInstance(fiber: Fiber): void,
prepareToHydrateHostInstance(fiber: Fiber, rootContainerInstance: C): boolean,
prepareToHydrateHostInstance(
fiber: Fiber,
rootContainerInstance: C,
hostContext: CX,
): boolean,
prepareToHydrateHostTextInstance(fiber: Fiber): boolean,
popHydrationState(fiber: Fiber): boolean,
};

module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
): HydrationContext<C> {
): HydrationContext<C, CX> {
const {
shouldSetTextContent,
canHydrateInstance,
Expand Down Expand Up @@ -218,13 +222,15 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
function prepareToHydrateHostInstance(
fiber: Fiber,
rootContainerInstance: C,
hostContext: CX,
): boolean {
const instance: I = fiber.stateNode;
const updatePayload = hydrateInstance(
instance,
fiber.type,
fiber.memoizedProps,
rootContainerInstance,
hostContext,
fiber,
);
// TODO: Type this specific to this type of component.
Expand Down
1 change: 1 addition & 0 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
View file
Open in desktop
Expand Up @@ -133,6 +133,7 @@ export type HostConfig<T, P, I, TI, PI, C, CX, PL> = {
type: T,
props: P,
rootContainerInstance: C,
hostContext: CX,
internalInstanceHandle: OpaqueHandle,
) => null | PL,
hydrateTextInstance?: (
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/fiber/ReactFiberScheduler.js
View file
Open in desktop
Expand Up @@ -152,7 +152,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
) {
const hostContext = ReactFiberHostContext(config);
const hydrationContext: HydrationContext<C> = ReactFiberHydrationContext(
const hydrationContext: HydrationContext<C, CX> = ReactFiberHydrationContext(
config,
);
const {popHostContainer, popHostContext, resetHostContainer} = hostContext;
Expand Down