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

Feature Request: Add Warning when consuming a context whose provider is not mounted #20003

Closed
jonrose-dev opened this issue Oct 12, 2020 · 11 comments

Comments

@jonrose-dev
Copy link

React allows you to consume a context, even if it's Provider is not mounted. Imagine a scenario where you are building a new context, and forget to add the Provider. When you consume it with either useContext or the Consumer component, React will run without issue, but your code won't work as expected. This has led to hours of lost time debugging the wrong problem. Adding a warning that a context is being consumed that does not have its Provider mounted, would help to reduce the time spent debugging this issue.

@dgocoder
Copy link

This is definitely a pain point

@Satsh24
Copy link

Satsh24 commented Oct 13, 2020

@jonrose-dev You can overcome that problem by following this pattern shown by Kent C Dodds. I found it so useful.

import React from 'react'
const CountStateContext = React.createContext()
const CountDispatchContext = React.createContext()

function CountProvider({children}) {
  const [count, setCount] = React.useState(0)
  return (
    <CountStateContext.Provider value={count}>
      <CountDispatchContext.Provider value={setCount}>
        {children}
      </CountDispatchContext.Provider>
    </CountStateContext.Provider>
  )
}
function useCountState() {
  const context = React.useContext(CountStateContext)
  if (context === undefined) {
    throw new Error('useCountState must be used within a CountProvider')
  }
  return context
}
function useCountDispatch() {
  const context = React.useContext(CountDispatchContext)
  if (context === undefined) {
    throw new Error('useCountDispatch must be used within a CountProvider')
  }
  return context
}
export {CountProvider, useCountState, useCountDispatch}

You can see the full article here

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Adding a warning that a context is being consumed that does not have its Provider mounted

FWIW reading a value from an unmounted context is a supported use case. (The context API supports a default value for this purpose.) For example, a UI library might export a theme context that allows you to override the component themes if you want or use the default theme if you don't.

Edit Maybe we could add a DEV warning only for contexts where createContext was called without any default value parameter?

@jonrose-dev
Copy link
Author

jonrose-dev commented Oct 13, 2020

@Satsh24 I'm actually using a similar pattern with the exception of the check for context. I think the pattern makes sense, but my concern is repeating the check for context in each hook is quite repetitive, and prone to the same pitfall of forgetfulness. I think the check for context existing should live inside useContext.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

@jonrose-dev, I accidentally clicked the edit button on your comment rather than mine to add the "edit" footnote. Sorry! It popped in under my comment and I guess I'm just not very good at GitHub still 😆

@jonrose-dev
Copy link
Author

@bvaughn interesting point I hadn't considered. I'd still love to see some sort of feedback telling me my Provider hasn't mounted when I attempt to use the context. Not sure if there is a best path forward then on this

@jonrose-dev
Copy link
Author

@jonrose-dev, I accidentally clicked the edit button on your comment rather than mine to add the "edit" footnote. Sorry! It popped in under my comment and I guess I'm just not very good at GitHub still 😆

lol no worries. I was confused at first but then figured it out 😆

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

I'd still love to see some sort of feedback telling me my Provider hasn't mounted when I attempt to use the context.

Well I mentioned above that maybe we could add a DEV warning only if the context was created without a default value (aka createContext())

@jonrose-dev
Copy link
Author

That would be helpful for those using JS and don't get the type checking for createContext. With TS, I already get this error.

Though this may not solve my specific problem (I use TS but just forget to mount the provider), I do agree it'd be helpful. I'd like to take a crack at the PR to add the warning

@asherccohen
Copy link

I quote the suggestion of a warning as in my experience it's hard to convince a team of the need for a safe check for missing provider. I did write one and a war started in my team on the reasons why I implemented it 🤣.
Would also like to see an optimized context that doesn't require useMemo on our values, but that's out of scope here.

@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

We intentionally don't do this because in many cases it's completely legit. In cases where it's not, you're welcome to implement this in userland. E.g. you can make some context null so that it crashes when unavailable. You can also, instead of exposing YourContext, expose a Hook like useYourContext. Inside that Hook, check if useContext(YourContext) gave you the default value (which you can make a special object). If yes, you can throw. That lets you implement the error or the warning with no involvement from React.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants