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

Deprecate context object as a consumer and add a warning message #13829

Merged
merged 14 commits into from
Oct 12, 2018
22 changes: 21 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type {ReactProviderType, ReactContext} from 'shared/ReactTypes';
import lowPriorityWarning from 'shared/lowPriorityWarning';
import type {Fiber} from 'react-reconciler/src/ReactFiber';
import type {FiberRoot} from './ReactFiberRoot';
import type {ExpirationTime} from './ReactFiberExpirationTime';
Expand Down Expand Up @@ -1096,12 +1097,31 @@ function updateContextProvider(
return workInProgress.child;
}

let hasWarnedAboutUsingContextAsConsumer = false;

function updateContextConsumer(
current: Fiber | null,
workInProgress: Fiber,
renderExpirationTime: ExpirationTime,
) {
const context: ReactContext<any> = workInProgress.type;
let context: ReactContext<any> = workInProgress.type;
if (__DEV__) {
if (workInProgress.type._context === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this just be context._context? To avoid extra reads (even in DEV).

if (!hasWarnedAboutUsingContextAsConsumer) {
hasWarnedAboutUsingContextAsConsumer = true;
lowPriorityWarning(
false,
'You are using the Context from React.createContext() as a consumer.' +
'The correct way is to use Context.Consumer as the consumer instead. ' +
'This usage is deprecated and will be removed in the next major version.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

"in a future major release"

);
}
} else {
// Context.Consumer is a separate object in DEV, where
// _context points back to the original context
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment confuses me. It talks about DEV but is in PROD code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a DEV path.

context = workInProgress.type._context;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why we read it differently in DEV and PROD? It would help to have a written explanation of DEV/PROD matrix for Consumer/Context object and how behavior changes for each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you reference me to another matrix? I updated the comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's none. I just meant if you could write up of what's supposed to change.

  • dev when using consumer: (before) and (after)
  • dev when using context: (before) and (after)
  • prod when using consumer: (before) and (after)
  • prod when using context: (before) and (after)

That would be easier for me to review since I can focus on intent separately from code.

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've put a big nice comment in there that explains things better now. I'll aadd the matrix to this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can also be just context._context.

}
}
const newProps = workInProgress.pendingProps;
const render = newProps.children;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1190,12 +1190,12 @@ describe('ReactNewContext', () => {

function FooAndBar() {
return (
<FooContext>
<FooContext.Consumer>
{foo => {
const bar = BarContext.unstable_read();
return <Text text={`Foo: ${foo}, Bar: ${bar}`} />;
}}
</FooContext>
</FooContext.Consumer>
);
}

Expand Down Expand Up @@ -1558,4 +1558,31 @@ Context fuzz tester error! Copy and paste the following line into the test suite
}
});
});

it('should warn with an error message when using context as a consumer in DEV', () => {
const BarContext = React.createContext({value: 'bar-initial'});
const BarConsumer = BarContext;

function Component() {
return (
<React.Fragment>
<BarContext.Provider value={{value: 'bar-updated'}}>
<BarConsumer>
{({value}) => <div actual={value} expected="bar-updated" />}
</BarConsumer>
</BarContext.Provider>
</React.Fragment>
);
}

expect(() => {
ReactNoop.render(<Component />);
ReactNoop.flush();
}).toLowPriorityWarnDev(
'You are using the Context from React.createContext() as a consumer.' +
'The correct way is to use Context.Consumer as the consumer instead. ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message might confuse people a little bit. Maybe make it more visual?

Rendering <Context> directly is not supported and will be removed in a future major release. Did you mean to render <Context.Consumer> instead?

Copy link

@ghost ghost May 26, 2019

Choose a reason for hiding this comment

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

This message is still not clear @trueadm . I'm already calling Context.Consumer as a consumer. But I'm still getting the error message.

import React from "react"

const defaultState = {
  dark: false,
  toggleDark: () => {},
}

const ThemeContext = Context.Consumer(defaultState)

// Getting dark mode information from OS!
// You need macOS Mojave + Safari Technology Preview Release 68 to test this currently.
const supportsDarkMode = () =>
  window.matchMedia("(prefers-color-scheme: dark)").matches === true

class ThemeProvider extends React.Component {
  state = {
    dark: false,
  }

  toggleDark = () => {
    let dark = !this.state.dark
    localStorage.setItem("dark", JSON.stringify(dark))
    this.setState({ dark })
  }

  componentDidMount() {
    // Getting dark mode value from localStorage!
    const lsDark = JSON.parse(localStorage.getItem("dark"))
    if (lsDark) {
      this.setState({ dark: lsDark })
    } else if (supportsDarkMode()) {
      this.setState({ dark: true })
    }
  }

  render() {
    const { children } = this.props
    const { dark } = this.state
    return (
      <ThemeContext.Provider
        value={{
          dark,
          toggleDark: this.toggleDark,
        }}
      >
        {children}
      </ThemeContext.Provider>
    )
  }
}

export default ThemeContext

export { ThemeProvider }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurosilvacom you should be using ThemeContext = createContext(...) and then, doing ThemeContext.Consumer and ThemeContext.Provider

Copy link

Choose a reason for hiding this comment

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

I'm not calling const ThemeContext = React.createContext(defaultState) but still getting the same error. Am I missing something about how I'm suppose to call <Context.Consumer> instead?

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 was suggesting you should call it and use it that way. I can help more when I’m back in the office in a few days.

Copy link

Choose a reason for hiding this comment

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

Sounds good. I appreciate the help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@laurosilvacom

I don't understand what you're trying to do.

You should create context like this:

const ThemeContext = React.createContext(defaultState) // NOT "Context.Consumer(defaultState)"

and then subscribe to it either like this:

class Button extends Component {
  static contextType = ThemeContext; // NOT ThemeContext.Consumer

  render() {
    // ...
  }
}

or like this:

class Button extends Component {
  render() {
    return (
      // NOT <ThemeContext>
      <ThemeContext.Consumer>
        {theme => ...}
      </ThemeContext.Consumer>
    );
  }
}

Does that help?

Copy link

@ghost ghost May 27, 2019

Choose a reason for hiding this comment

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

Yes!

I did not realize I needed to subscribe to it by calling <ThemeContext.Consumer>.

I had:

 <ThemeContext>
        {theme => ...}
 </ThemeContext>

Thanks @gaearon!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, which is why the warning says:

Rendering <Context> directly is not supported and will be removed in a future major release. Did you mean to render <Context.Consumer> instead?

Do you still feel it was unclear? Which part could be improved? Thanks!

Copy link

@ghost ghost May 27, 2019

Choose a reason for hiding this comment

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

An error message that's direct would help.

Rendering directly is not supported and will be removed in a future major release. You want to use <Context.Consumer> instead.

'This usage is deprecated and will be removed in the next major version.',
{withoutStack: true},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why no stack? This is exactly the kind of warning where I think the stack would be highly valuable.

Copy link
Contributor Author

@trueadm trueadm Oct 12, 2018

Choose a reason for hiding this comment

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

n00b question: how does one generate a stack with lowPriorityWarning? I know you can with warning but then the APIs are consistent then. I'll just switch to warning to unblock this.

);
});
});
41 changes: 40 additions & 1 deletion packages/react/src/ReactContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,46 @@ export function createContext<T>(
$$typeof: REACT_PROVIDER_TYPE,
_context: context,
};
context.Consumer = context;

if (__DEV__) {
// A separate object, but proxies back to the original context object for
// backwards compatibility. It has a different $$typeof, so we can properly
// warn for the incorrect usage of Context as a Consumer.
context.Consumer = {
$$typeof: REACT_CONTEXT_TYPE,
_context: context,
get _calculateChangedBits() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think getters don't work on www, can you rewrite this as Object.defineProperty calls?
Worth checking tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? They work in IE9 the last time I checked.

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 changed over to Object.defineProperties. It's a shame Flow has such bad support for them though :(

return context._calculateChangedBits;
},
set _calculateChangedBits(_calculateChangedBits) {
context._calculateChangedBits = _calculateChangedBits;
},
get _currentValue() {
return context._currentValue;
},
set _currentValue(_currentValue) {
context._currentValue = _currentValue;
},
get _currentValue2() {
return context._currentValue2;
},
set _currentValue2(_currentValue2) {
context._currentValue2 = _currentValue2;
},
Provider: context.Provider,
get Consumer() {
return context.Consumer;
},
get unstable_read() {
return context.unstable_read;
},
set unstable_read(unstable_read) {
context.unstable_read = unstable_read;
},
};
} else {
context.Consumer = context;
}
context.unstable_read = readContext.bind(null, context);

if (__DEV__) {
Expand Down