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.

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

Patch release branch for 16.8.3 #14902

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -482,5 +482,98 @@ describe('ReactDOMServerIntegration', () => {
);
}
});

// Regression test for https://github.com/facebook/react/issues/14705
it('does not pollute later renders when stream destroyed', () => {
const LoggedInUser = React.createContext('default');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
AppWithUser('Amy'),
).setEncoding('utf8');

// This is an implementation detail because we test a memory leak
const {threadID} = stream.partialRenderer;

// Read enough to render Provider but not enough for it to be exited
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

stream.destroy();

const AppWithUserNoProvider = () => (
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
);

const stream2 = ReactDOMServer.renderToNodeStream(
AppWithUserNoProvider(),
).setEncoding('utf8');

// Sanity check to ensure 2nd render has same threadID as 1st render,
// otherwise this test is not testing what it's meant to
expect(stream2.partialRenderer.threadID).toBe(threadID);

const markup = stream2.read(Infinity);

expect(markup).toBe('default');
});

// Regression test for https://github.com/facebook/react/issues/14705
it('frees context value reference when stream destroyed', () => {
const LoggedInUser = React.createContext('default');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
AppWithUser('Amy'),
).setEncoding('utf8');

// This is an implementation detail because we test a memory leak
const {threadID} = stream.partialRenderer;

// Read enough to render Provider but not enough for it to be exited
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

stream.destroy();
expect(LoggedInUser[threadID]).toBe('default');
});

it('does not pollute sync renders after an error', () => {
const LoggedInUser = React.createContext('default');
const Crash = () => {
throw new Error('Boo!');
};
const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
<Crash />
</LoggedInUser.Provider>
);

expect(() => {
ReactDOMServer.renderToString(AppWithUser('Casper'));
}).toThrow('Boo');

// Should not report a value from failed render
expect(
ReactDOMServer.renderToString(
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>,
),
).toBe('default');
});
});
});
10 changes: 10 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ class ReactDOMServerRenderer {
destroy() {
if (!this.exhausted) {
this.exhausted = true;
this.clearProviders();
freeThreadID(this.threadID);
}
}
Expand Down Expand Up @@ -776,6 +777,15 @@ class ReactDOMServerRenderer {
context[this.threadID] = previousValue;
}

clearProviders(): void {
// Restore any remaining providers on the stack to previous values
for (let index = this.contextIndex; index >= 0; index--) {
const context: ReactContext<any> = this.contextStack[index];
const previousValue = this.contextValueStack[index];
context[this.threadID] = previousValue;
}
}

read(bytes: number): string | null {
if (this.exhausted) {
return null;
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,6 @@ function updateReducer<S, I, A>(
}

hook.memoizedState = newState;

// Don't persist the state accumlated from the render phase updates to
// the base state unless the queue is empty.
// TODO: Not sure if this is the desired semantics, but it's what we
Expand All @@ -616,6 +615,9 @@ function updateReducer<S, I, A>(
hook.baseState = newState;
}

queue.eagerReducer = reducer;
queue.eagerState = newState;

return [newState, dispatch];
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,76 @@ describe('ReactHooks', () => {
}).toThrow('is not a function');
});

it('does not forget render phase useState updates inside an effect', () => {
const {useState, useEffect} = React;

function Counter() {
const [counter, setCounter] = useState(0);
if (counter === 0) {
setCounter(x => x + 1);
setCounter(x => x + 1);
}
useEffect(() => {
setCounter(x => x + 1);
setCounter(x => x + 1);
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('does not forget render phase useReducer updates inside an effect with hoisted reducer', () => {
const {useReducer, useEffect} = React;

const reducer = x => x + 1;
function Counter() {
const [counter, increment] = useReducer(reducer, 0);
if (counter === 0) {
increment();
increment();
}
useEffect(() => {
increment();
increment();
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('does not forget render phase useReducer updates inside an effect with inline reducer', () => {
const {useReducer, useEffect} = React;

function Counter() {
const [counter, increment] = useReducer(x => x + 1, 0);
if (counter === 0) {
increment();
increment();
}
useEffect(() => {
increment();
increment();
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('warns for bad useImperativeHandle first arg', () => {
const {useImperativeHandle} = React;
function App() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,9 @@ describe('ReactHooksWithNoopRenderer', () => {

// Test that it works on update, too. This time the log is a bit different
// because we started with reducerB instead of reducerA.
counter.current.dispatch('reset');
ReactNoop.act(() => {
counter.current.dispatch('reset');
});
ReactNoop.render(<Counter ref={counter} />);
expect(ReactNoop.flush()).toEqual([
'Render: 0',
Expand Down