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

Show deprecated context object warnings usage in ReactDOM server #14033

Merged
merged 6 commits into from
Nov 7, 2018
Merged

Show deprecated context object warnings usage in ReactDOM server #14033

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9262995
Applies context object warnings to ReactDOM server
trueadm Oct 30, 2018
7514a7b
fix flow
trueadm Oct 30, 2018
49346bd
Merge remote-tracking branch 'upstream/master' into ssr-context-objec…
trueadm Nov 5, 2018
f533a4b
Fix eslint
trueadm Nov 5, 2018
0afeb07
run prettier
trueadm Nov 6, 2018
65cab46
address feedback
trueadm Nov 6, 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
99 changes: 99 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,5 +249,104 @@ describe('ReactDOMServerIntegration', () => {
expect(e.querySelector('#language2').textContent).toBe('sanskrit');
expect(e.querySelector('#language3').textContent).toBe('french');
});

itRenders(
'should warn with an error message when using Context as consumer in DEV',
async render => {
const Theme = React.createContext('dark');
const Language = React.createContext('french');

const App = () => (
<div>
<Theme.Provider value="light">
<Language.Provider value="english">
<Theme.Provider value="dark">
<Theme>{theme => <div id="theme1">{theme}</div>}</Theme>
</Theme.Provider>
</Language.Provider>
</Theme.Provider>
</div>
);
// We expect 1 error.
await render(<App />, 1);
},
);

// False positive regression test.
itRenders(
'should not warn when using Consumer from React < 16.6 with newer renderer',
async render => {
const Theme = React.createContext('dark');
const Language = React.createContext('french');
// React 16.5 and earlier didn't have a separate object.
Theme.Consumer = Theme;

const App = () => (
<div>
<Theme.Provider value="light">
<Language.Provider value="english">
<Theme.Provider value="dark">
<Theme>{theme => <div id="theme1">{theme}</div>}</Theme>
</Theme.Provider>
</Language.Provider>
</Theme.Provider>
</div>
);
// We expect 0 errors.
await render(<App />, 0);
},
);

itRenders(
'should warn with an error message when using nested context consumers in DEV',
async render => {
const App = () => {
const Theme = React.createContext('dark');
const Language = React.createContext('french');

return (
<div>
<Theme.Provider value="light">
<Language.Provider value="english">
<Theme.Provider value="dark">
<Theme.Consumer.Consumer>
{theme => <div id="theme1">{theme}</div>}
</Theme.Consumer.Consumer>
</Theme.Provider>
</Language.Provider>
</Theme.Provider>
</div>
);
};
// We expect 1 error.
await render(<App />, 1);
},
);

itRenders(
'should warn with an error message when using Context.Consumer.Provider DEV',
async render => {
const App = () => {
const Theme = React.createContext('dark');
const Language = React.createContext('french');

return (
<div>
<Theme.Provider value="light">
<Language.Provider value="english">
<Theme.Consumer.Provider value="dark">
<Theme.Consumer>
{theme => <div id="theme1">{theme}</div>}
</Theme.Consumer>
</Theme.Consumer.Provider>
</Language.Provider>
</Theme.Provider>
</div>
);
};
// We expect 1 error.
await render(<App />, 1);
},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests seem a little weak since they don't assert on what the error message actually is. I guess ReactDOMServerIntegrationTestUtils doesn't provide a mechanism to do this currently, but it seems like it would be an improvement if e.g. expectErrors supported either a number or an error of partial messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this is something I stumbled into. It can be something we do as a follow up as I know that we're planning on making some changes to the server renderer and this might flow nicely with that workload.

});
});
34 changes: 30 additions & 4 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 @@ -10,7 +10,6 @@
import type {ReactElement} from 'shared/ReactElementType';
import type {
ReactProvider,
ReactConsumer,
ReactContext,
} from 'shared/ReactTypes';

Expand Down Expand Up @@ -91,6 +90,7 @@ let validatePropertiesInDevelopment = (type, props) => {};
let pushCurrentDebugStack = (stack: Array<Frame>) => {};
let pushElementToDebugStack = (element: ReactElement) => {};
let popCurrentDebugStack = () => {};
let hasWarnedAboutUsingContextAsConsumer = false;

if (__DEV__) {
ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;
Expand Down Expand Up @@ -1054,9 +1054,35 @@ class ReactDOMServerRenderer {
return '';
}
case REACT_CONTEXT_TYPE: {
const consumer: ReactConsumer<any> = (nextChild: any);
const nextProps: any = consumer.props;
const nextValue = consumer.type._currentValue;
let reactContext: ReactContext<any> = (nextChild: any).type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a little odd that we're any-casting something that explicitly might not be ReactContext to that type for DEV mode.

// The logic below for Context differs depending on PROD or DEV mode. In
// DEV mode, we create a separate object for Context.Consumer that acts
// like a proxy to Context. This proxy object adds unnecessary code in PROD
// so we use the old behaviour (Context.Consumer references Context) to
// reduce size and overhead. The separate object references context via
// a property called "_context", which also gives us the ability to check
// in DEV mode if this property exists or not and warn if it does not.
if (__DEV__) {
if ((reactContext: any)._context === undefined) {
// This may be because it's a Context (rather than a Consumer).
// Or it may be because it's older React where they're the same thing.
// We only want to warn if we're sure it's a new React.
if (reactContext !== reactContext.Consumer) {
if (!hasWarnedAboutUsingContextAsConsumer) {
hasWarnedAboutUsingContextAsConsumer = true;
warning(
false,
'Rendering <Context> directly is not supported and will be removed in ' +
'a future major release. Did you mean to render <Context.Consumer> instead?',
);
}
}
} else {
reactContext = (reactContext: any)._context;
}
}
const nextProps: any = (nextChild: any).props;
const nextValue = reactContext._currentValue;

const nextChildren = toArray(nextProps.children(nextValue));
const frame: Frame = {
Expand Down