Skip to content

Add support for re-entrant SSR stacks #13181

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

Merged
gaearon merged 2 commits into from
Jul 11, 2018
+205 −42
Merged

Add support for re-entrant SSR stacks #13181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ecbb92
Add failing tests
gaearon Jul 11, 2018
503ddd8
Fix re-entrancy in ReactDOMServer
gaearon Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 72 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOM-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,20 @@

'use strict';

let React = require('react');
let ReactDOM = require('react-dom');
const ReactTestUtils = require('react-dom/test-utils');
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

describe('ReactDOM', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
});

// TODO: uncomment this test once we can run in phantom, which
// supports real submit events.
/*
Expand Down Expand Up @@ -446,4 +455,64 @@ describe('ReactDOM', () => {
global.requestAnimationFrame = previousRAF;
}
});

it('reports stacks with re-entrant renderToString() calls on the client', () => {
function Child2(props) {
return <span ariaTypo3="no">{props.children}</span>;
}

function App2() {
return (
<Child2>
{ReactDOMServer.renderToString(<blink ariaTypo2="no" />)}
</Child2>
);
}

function Child() {
return (
<span ariaTypo4="no">{ReactDOMServer.renderToString(<App2 />)}</span>
);
}

function ServerEntry() {
return ReactDOMServer.renderToString(<Child />);
}

function App() {
return (
<div>
<span ariaTypo="no" />
<ServerEntry />
<font ariaTypo5="no" />
</div>
);
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<App />, container)).toWarnDev([
// ReactDOM(App > div > span)
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
// ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink)
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in blink (at **)',
// ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child) >>> ReactDOMServer(App2 > Child2 > span)
'Invalid ARIA attribute `ariaTypo3`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child2 (at **)\n' +
' in App2 (at **)',
// ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child > span)
'Invalid ARIA attribute `ariaTypo4`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child (at **)',
// ReactDOM(App > div > font)
'Invalid ARIA attribute `ariaTypo5`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in font (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
]);
});
});
57 changes: 57 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,4 +720,61 @@ describe('ReactDOMServer', () => {
' in App (at **)',
]);
});

it('reports stacks with re-entrant renderToString() calls', () => {
function Child2(props) {
return <span ariaTypo3="no">{props.children}</span>;
}

function App2() {
return (
<Child2>
{ReactDOMServer.renderToString(<blink ariaTypo2="no" />)}
</Child2>
);
}

function Child() {
return (
<span ariaTypo4="no">{ReactDOMServer.renderToString(<App2 />)}</span>
);
}

function App() {
return (
<div>
<span ariaTypo="no" />
<Child />
<font ariaTypo5="no" />
</div>
);
}

expect(() => ReactDOMServer.renderToString(<App />)).toWarnDev([
// ReactDOMServer(App > div > span)
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
// ReactDOMServer(App > div > Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink)
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in blink (at **)',
// ReactDOMServer(App > div > Child) >>> ReactDOMServer(App2 > Child2 > span)
'Invalid ARIA attribute `ariaTypo3`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child2 (at **)\n' +
' in App2 (at **)',
// ReactDOMServer(App > div > Child > span)
'Invalid ARIA attribute `ariaTypo4`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
// ReactDOMServer(App > div > font)
'Invalid ARIA attribute `ariaTypo5`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in font (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
]);
});
Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 11, 2018

Choose a reason for hiding this comment

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

Nice tests 👍

});
115 changes: 76 additions & 39 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,20 @@ type FlatReactChildren = Array<null | ReactNode>;
type toArrayType = (children: mixed) => FlatReactChildren;
const toArray = ((React.Children.toArray: any): toArrayType);

let currentDebugStack;
let currentDebugElementStack;

let getStackAddendum = () => '';
// This is only used in DEV.
// Each entry is `this.stack` from a currently executing renderer instance.
// (There may be more than one because ReactDOMServer is reentrant).
// Each stack is an array of frames which may contain nested stacks of elements.
let currentDebugStacks = [];

let prevGetCurrentStackImpl = null;
let getCurrentServerStackImpl = () => '';
let describeStackFrame = element => '';

let validatePropertiesInDevelopment = (type, props) => {};
let setCurrentDebugStack = (stack: Array<Frame>) => {};
let pushCurrentDebugStack = (stack: Array<Frame>) => {};
let pushElementToDebugStack = (element: ReactElement) => {};
let resetCurrentDebugStack = () => {};
let popCurrentDebugStack = () => {};

if (__DEV__) {
validatePropertiesInDevelopment = function(type, props) {
Expand All @@ -87,34 +91,55 @@ if (__DEV__) {
return describeComponentFrame(name, source, ownerName);
};

currentDebugStack = null;
currentDebugElementStack = null;
setCurrentDebugStack = function(stack: Array<Frame>) {
const frame: Frame = stack[stack.length - 1];
currentDebugElementStack = ((frame: any): FrameDev).debugElementStack;
// We are about to enter a new composite stack, reset the array.
currentDebugElementStack.length = 0;
currentDebugStack = stack;
ReactDebugCurrentFrame.getCurrentStack = getStackAddendum;
pushCurrentDebugStack = function(stack: Array<Frame>) {
currentDebugStacks.push(stack);

if (currentDebugStacks.length === 1) {
// We are entering a server renderer.
// Remember the previous (e.g. client) global stack implementation.
prevGetCurrentStackImpl = ReactDebugCurrentFrame.getCurrentStack;
Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 11, 2018

Choose a reason for hiding this comment

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

Huh... does this work? Do we never push more than once before popping?

Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 11, 2018

Choose a reason for hiding this comment

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

Hmm... or is this just assuming that all pushes before the pop will be for the same renderer and will have the same previous stack impl? I find this more confusing initially 😆

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 11, 2018

Choose a reason for hiding this comment

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

It's assuming that once you're inside ReactDOMServer call, you're only going to make other ReactDOMServer calls (at most), but not, e.g., ReactDOMClient or ReactART calls. Because it's synchronous. I think it's a safe assumption in 99% cases.

All ReactDOMServer calls share the same implementation. Happy to talk through it via VC — I'm worried this is getting difficult to explain like this.

Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 11, 2018

Choose a reason for hiding this comment

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

Gotcha. Yeah, that's what I was thinking from reading it. Was just unsure if that would ever cause problems.

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Jul 11, 2018

Choose a reason for hiding this comment

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

In the worst case you'll end up with no stacks. Which is exactly what's happening today with any nesting. :-)

Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jul 11, 2018

Choose a reason for hiding this comment

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

Fair enough!

ReactDebugCurrentFrame.getCurrentStack = getCurrentServerStackImpl;
}
};

pushElementToDebugStack = function(element: ReactElement) {
if (currentDebugElementStack !== null) {
currentDebugElementStack.push(element);
}
// For the innermost executing ReactDOMServer call,
const stack = currentDebugStacks[currentDebugStacks.length - 1];
// Take the innermost executing frame (e.g. <Foo>),
const frame: Frame = stack[stack.length - 1];
// and record that it has one more element associated with it.
((frame: any): FrameDev).debugElementStack.push(element);
// We only need this because we tail-optimize single-element
// children and directly handle them in an inner loop instead of
// creating separate frames for them.
};
resetCurrentDebugStack = function() {
currentDebugElementStack = null;
currentDebugStack = null;
ReactDebugCurrentFrame.getCurrentStack = null;

popCurrentDebugStack = function() {
currentDebugStacks.pop();

if (currentDebugStacks.length === 0) {
// We are exiting the server renderer.
// Restore the previous (e.g. client) global stack implementation.
ReactDebugCurrentFrame.getCurrentStack = prevGetCurrentStackImpl;
prevGetCurrentStackImpl = null;
}
};
getStackAddendum = function(): null | string {
if (currentDebugStack === null) {

getCurrentServerStackImpl = function(): string {
if (currentDebugStacks.length === 0) {
// Nothing is currently rendering.
return '';
}
// ReactDOMServer is reentrant so there may be multiple calls at the same time.
// Take the frames from the innermost call which is the last in the array.
let frames = currentDebugStacks[currentDebugStacks.length - 1];
let stack = '';
let debugStack = currentDebugStack;
for (let i = debugStack.length - 1; i >= 0; i--) {
const frame: Frame = debugStack[i];
// Go through every frame in the stack from the innermost one.
for (let i = frames.length - 1; i >= 0; i--) {
const frame: Frame = frames[i];
// Every frame might have more than one debug element stack entry associated with it.
// This is because single-child nesting doesn't create materialized frames.
// Instead it would push them through `pushElementToDebugStack()`.
let debugElementStack = ((frame: any): FrameDev).debugElementStack;
for (let ii = debugElementStack.length - 1; ii >= 0; ii--) {
stack += describeStackFrame(debugElementStack[ii]);
Expand Down Expand Up @@ -180,7 +205,7 @@ function createMarkupForStyles(styles): string | null {
const styleValue = styles[styleName];
if (__DEV__) {
if (!isCustomProperty) {
warnValidStyle(styleName, styleValue, getStackAddendum);
warnValidStyle(styleName, styleValue, getCurrentServerStackImpl);
}
}
if (styleValue != null) {
Expand Down Expand Up @@ -305,7 +330,13 @@ function maskContext(type, context) {

function checkContextTypes(typeSpecs, values, location: string) {
if (__DEV__) {
checkPropTypes(typeSpecs, values, location, 'Component', getStackAddendum);
checkPropTypes(
typeSpecs,
values,
location,
'Component',
getCurrentServerStackImpl,
);
}
}

Expand Down Expand Up @@ -774,12 +805,18 @@ class ReactDOMServerRenderer {
}
const child = frame.children[frame.childIndex++];
if (__DEV__) {
setCurrentDebugStack(this.stack);
}
out += this.render(child, frame.context, frame.domNamespace);
if (__DEV__) {
// TODO: Handle reentrant server render calls. This doesn't.
resetCurrentDebugStack();
pushCurrentDebugStack(this.stack);
// We're starting work on this frame, so reset its inner stack.
((frame: any): FrameDev).debugElementStack.length = 0;
try {
// Be careful! Make sure this matches the PROD path below.
out += this.render(child, frame.context, frame.domNamespace);
} finally {
popCurrentDebugStack();
}
} else {
// Be careful! Make sure this matches the DEV path above.
out += this.render(child, frame.context, frame.domNamespace);
}
}
return out;
Expand Down Expand Up @@ -1005,7 +1042,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'input',
props,
getStackAddendum,
getCurrentServerStackImpl,
);

if (
Expand Down Expand Up @@ -1063,7 +1100,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'textarea',
props,
getStackAddendum,
getCurrentServerStackImpl,
);
if (
props.value !== undefined &&
Expand Down Expand Up @@ -1124,7 +1161,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'select',
props,
getStackAddendum,
getCurrentServerStackImpl,
);

for (let i = 0; i < valuePropNames.length; i++) {
Expand Down Expand Up @@ -1215,7 +1252,7 @@ class ReactDOMServerRenderer {
validatePropertiesInDevelopment(tag, props);
}

assertValidProps(tag, props, getStackAddendum);
assertValidProps(tag, props, getCurrentServerStackImpl);

let out = createOpenTagMarkup(
element.type,
Expand Down