From 8ff3f6230eb63737321e2a86fd533afb672b03dc Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Tue, 5 Jun 2018 15:30:42 -0700 Subject: [PATCH 1/8] Fixed an issue with nested contexts unwinding when server rendering. GitHub issue #12984 --- ...eactDOMServerIntegrationNewContext-test.js | 28 +++++++++++++++++++ .../src/server/ReactPartialRenderer.js | 21 +++++++++----- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index c3c08eb6bf773..9acfaa979707d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -191,5 +191,33 @@ describe('ReactDOMServerIntegration', () => { expect(e.querySelector('#theme').textContent).toBe('light'); expect(e.querySelector('#language').textContent).toBe('english'); }); + + itRenders( + 'nested context unwinding', + async render => { + const Theme = React.createContext('dark'); + const Language = React.createContext('french'); + + const App = () => ( +
+ + + + + {theme =>
{theme}
} +
+
+ + {theme =>
{theme}
} +
+
+
+
+ ); + let e = await render(); + expect(e.querySelector('#theme1').textContent).toBe('dark'); + expect(e.querySelector('#theme2').textContent).toBe('light'); + }, + ); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 59ea170b938d5..98b463384da70 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -689,14 +689,21 @@ class ReactDOMServerRenderer { this.providerStack[this.providerIndex] = null; this.providerIndex -= 1; const context: ReactContext = provider.type._context; - if (this.providerIndex < 0) { - context._currentValue = context._defaultValue; - } else { - // We assume this type is correct because of the index check above. - const previousProvider: ReactProvider = (this.providerStack[ - this.providerIndex - ]: any); + // find the correct previous provider based on type + let previousProvider; + if (this.providerIndex > -1) { + for (let i = 0; i <= this.providerIndex; i += 1) { + if (this.providerStack[i] && + (this.providerStack[i]: ReactProvider).type === provider.type) { + previousProvider = this.providerStack[i]; + break; + } + } + } + if (previousProvider) { context._currentValue = previousProvider.props.value; + } else { + context._currentValue = context._defaultValue; } } From 27bb3299964d5c7e95e849037973b61b2b6d329a Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Tue, 5 Jun 2018 16:14:36 -0700 Subject: [PATCH 2/8] Fixed an issue with search direction and stricter false checking --- packages/react-dom/src/server/ReactPartialRenderer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 98b463384da70..9acbf95bc62c9 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -692,8 +692,8 @@ class ReactDOMServerRenderer { // find the correct previous provider based on type let previousProvider; if (this.providerIndex > -1) { - for (let i = 0; i <= this.providerIndex; i += 1) { - if (this.providerStack[i] && + for (let i = this.providerIndex; i >= 0; i -= 1) { + if (this.providerStack[i] !== null && this.providerStack[i] !== undefined && (this.providerStack[i]: ReactProvider).type === provider.type) { previousProvider = this.providerStack[i]; break; From 8d38f45796643a6da2e85f74d65b3a7e96b67fd1 Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Tue, 5 Jun 2018 16:20:10 -0700 Subject: [PATCH 3/8] Use decrement infix operator --- packages/react-dom/src/server/ReactPartialRenderer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 9acbf95bc62c9..c6d83760cc9ea 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -692,7 +692,7 @@ class ReactDOMServerRenderer { // find the correct previous provider based on type let previousProvider; if (this.providerIndex > -1) { - for (let i = this.providerIndex; i >= 0; i -= 1) { + for (let i = this.providerIndex; i >= 0; i--) { if (this.providerStack[i] !== null && this.providerStack[i] !== undefined && (this.providerStack[i]: ReactProvider).type === provider.type) { previousProvider = this.providerStack[i]; From 037c4d5f61674d11a6cf1ce4e78f37cf5c1341e6 Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Tue, 5 Jun 2018 16:54:31 -0700 Subject: [PATCH 4/8] Streamlined existence checks --- .../src/server/ReactPartialRenderer.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index c6d83760cc9ea..a3b1a86f8a05e 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -641,7 +641,7 @@ class ReactDOMServerRenderer { previousWasTextNode: boolean; makeStaticMarkup: boolean; - providerStack: Array>; + providerStack: Array>; providerIndex: number; constructor(children: mixed, makeStaticMarkup: boolean) { @@ -686,21 +686,19 @@ class ReactDOMServerRenderer { 'Unexpected pop.', ); } - this.providerStack[this.providerIndex] = null; + this.providerStack.length = this.providerIndex; this.providerIndex -= 1; const context: ReactContext = provider.type._context; // find the correct previous provider based on type - let previousProvider; - if (this.providerIndex > -1) { - for (let i = this.providerIndex; i >= 0; i--) { - if (this.providerStack[i] !== null && this.providerStack[i] !== undefined && - (this.providerStack[i]: ReactProvider).type === provider.type) { - previousProvider = this.providerStack[i]; - break; - } + let previousProvider = null; + for (let i = this.providerIndex; i >= 0; i--) { + // We assume this Flow type is correct because of the index check above. + if ((this.providerStack[i]: ReactProvider).type === provider.type) { + previousProvider = this.providerStack[i]; + break; } } - if (previousProvider) { + if (previousProvider !== null) { context._currentValue = previousProvider.props.value; } else { context._currentValue = context._defaultValue; From e69904c2480dfee13b5b5ee6b42df67f7faad88b Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Tue, 5 Jun 2018 17:57:19 -0700 Subject: [PATCH 5/8] Streamlined assignment. Removed redundant comment. Use null for array values --- .../src/server/ReactPartialRenderer.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index a3b1a86f8a05e..4f660724134b7 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -641,7 +641,7 @@ class ReactDOMServerRenderer { previousWasTextNode: boolean; makeStaticMarkup: boolean; - providerStack: Array>; + providerStack: Array>; providerIndex: number; constructor(children: mixed, makeStaticMarkup: boolean) { @@ -686,20 +686,21 @@ class ReactDOMServerRenderer { 'Unexpected pop.', ); } - this.providerStack.length = this.providerIndex; + this.providerStack[this.providerIndex] = null; this.providerIndex -= 1; const context: ReactContext = provider.type._context; - // find the correct previous provider based on type - let previousProvider = null; + let contextPriorProvider = null; for (let i = this.providerIndex; i >= 0; i--) { // We assume this Flow type is correct because of the index check above. - if ((this.providerStack[i]: ReactProvider).type === provider.type) { - previousProvider = this.providerStack[i]; + const priorProvider: ?ReactProvider = this.providerStack[i]; + if (priorProvider !== null && priorProvider !== undefined && + priorProvider.type === provider.type) { + contextPriorProvider = priorProvider; break; } } - if (previousProvider !== null) { - context._currentValue = previousProvider.props.value; + if (contextPriorProvider !== null) { + context._currentValue = contextPriorProvider.props.value; } else { context._currentValue = context._defaultValue; } From 731c686df3babc0122ab92b5c00e45ad54100e5d Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Wed, 6 Jun 2018 13:31:26 -0700 Subject: [PATCH 6/8] Made prettier --- ...eactDOMServerIntegrationNewContext-test.js | 45 +++++++++---------- .../src/server/ReactPartialRenderer.js | 7 ++- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index 9acfaa979707d..5c934f5986727 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -192,32 +192,29 @@ describe('ReactDOMServerIntegration', () => { expect(e.querySelector('#language').textContent).toBe('english'); }); - itRenders( - 'nested context unwinding', - async render => { - const Theme = React.createContext('dark'); - const Language = React.createContext('french'); + itRenders('nested context unwinding', async render => { + const Theme = React.createContext('dark'); + const Language = React.createContext('french'); - const App = () => ( -
- - - - - {theme =>
{theme}
} -
-
+ const App = () => ( +
+ + + - {theme =>
{theme}
} + {theme =>
{theme}
}
-
-
-
- ); - let e = await render(); - expect(e.querySelector('#theme1').textContent).toBe('dark'); - expect(e.querySelector('#theme2').textContent).toBe('light'); - }, - ); +
+ + {theme =>
{theme}
} +
+ + +
+ ); + let e = await render(); + expect(e.querySelector('#theme1').textContent).toBe('dark'); + expect(e.querySelector('#theme2').textContent).toBe('light'); + }); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 4f660724134b7..b4da2b8a0cf66 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -693,8 +693,11 @@ class ReactDOMServerRenderer { for (let i = this.providerIndex; i >= 0; i--) { // We assume this Flow type is correct because of the index check above. const priorProvider: ?ReactProvider = this.providerStack[i]; - if (priorProvider !== null && priorProvider !== undefined && - priorProvider.type === provider.type) { + if ( + priorProvider !== null && + priorProvider !== undefined && + priorProvider.type === provider.type + ) { contextPriorProvider = priorProvider; break; } From 6d31d3a5632993de43af9d3b507db54850c86ea3 Mon Sep 17 00:00:00 2001 From: Eric Soderberg Date: Wed, 6 Jun 2018 16:03:07 -0700 Subject: [PATCH 7/8] Relaxed type checking and improved comment --- packages/react-dom/src/server/ReactPartialRenderer.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index b4da2b8a0cf66..3f794020c0fce 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -691,13 +691,10 @@ class ReactDOMServerRenderer { const context: ReactContext = provider.type._context; let contextPriorProvider = null; for (let i = this.providerIndex; i >= 0; i--) { - // We assume this Flow type is correct because of the index check above. - const priorProvider: ?ReactProvider = this.providerStack[i]; - if ( - priorProvider !== null && - priorProvider !== undefined && - priorProvider.type === provider.type - ) { + // We assume this Flow type is correct because of the index check above + // and because pushProvider() enforces the correct type. + const priorProvider: any = this.providerStack[i]; + if (priorProvider.type === provider.type) { contextPriorProvider = priorProvider; break; } From 5114a3e506d2a7921a0ded0c08de793a8f585d27 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Jun 2018 14:12:51 +0100 Subject: [PATCH 8/8] Improve test coverage --- ...eactDOMServerIntegrationNewContext-test.js | 35 ++++++++++++++++++- .../src/server/ReactPartialRenderer.js | 5 ++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index 5c934f5986727..2bcece083a4b8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -199,7 +199,7 @@ describe('ReactDOMServerIntegration', () => { const App = () => (
- + {theme =>
{theme}
} @@ -208,13 +208,46 @@ describe('ReactDOMServerIntegration', () => { {theme =>
{theme}
}
+ + + + + {() => ( + + + + {language =>
{language}
} +
+
+ )} +
+
+ + {language => ( + + + {theme =>
{theme}
} +
+
{language}
+
+ )} +
+
+
+ + {language =>
{language}
} +
); let e = await render(); expect(e.querySelector('#theme1').textContent).toBe('dark'); expect(e.querySelector('#theme2').textContent).toBe('light'); + expect(e.querySelector('#theme3').textContent).toBe('blue'); + expect(e.querySelector('#language1').textContent).toBe('chinese'); + expect(e.querySelector('#language2').textContent).toBe('sanskrit'); + expect(e.querySelector('#language3').textContent).toBe('french'); }); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 3f794020c0fce..f33ffd239eb2a 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -689,11 +689,14 @@ class ReactDOMServerRenderer { this.providerStack[this.providerIndex] = null; this.providerIndex -= 1; const context: ReactContext = provider.type._context; + + // Find the closest parent provider of the same type and use its value. + // TODO: it would be nice to avoid this being O(N). let contextPriorProvider = null; for (let i = this.providerIndex; i >= 0; i--) { // We assume this Flow type is correct because of the index check above // and because pushProvider() enforces the correct type. - const priorProvider: any = this.providerStack[i]; + const priorProvider: ReactProvider = (this.providerStack[i]: any); if (priorProvider.type === provider.type) { contextPriorProvider = priorProvider; break;