fix: Throw a better error message if context is null #192
Conversation
|
Thanks! I think this makes sense. However, some of your changes makes the compiler unhappy. Could you have a look? |
|
Will do! |
|
@FredrikOseberg Updated the tests |
| @@ -17,6 +17,6 @@ export interface IFlagContextValue | |||
| >; | |||
| } | |||
|
|
|||
| const FlagContext = React.createContext<IFlagContextValue>(null as never); | |||
| const FlagContext = React.createContext<IFlagContextValue | null>(null); | |||
There was a problem hiding this comment.
This line is a bit concerning, because if anyone is using the FlagContext directly they would receive the same compiler errors as we had in the test. Ideally, I'd love for this change to not require a major update and this is the only part that is changing the public facing API in a way that might break clients.
There was a problem hiding this comment.
I would argue that the types were lying to the user before. The flag context is defaulted to null, but the type system is telling users that it will always have a value. This is why the error thrown if you forget to wrap your application in the provider is about the context being null.
If you want we can maybe re-cast the public export so it's not a breaking change? I think for the internal logic of this library it's important that the context is potentially null.
I was thinking something like this in the index.ts file
import FlagContext from './FlagContext';
const FlagContextWithoutNull = FlagContext as React.Context<IFlagContextValue | null>
export {
FlagContextWithoutNull as FlagContext,
// other exports
}and then you could clean that up in a future major update
There was a problem hiding this comment.
I think you make a good point. And looking at our releases it's been a while since the last major. I will merge this as is and release it as 5.0.0. If we leave it I'm afraid we'll never get around to it.
About the changes
If a developer forgets to render their hooks inside of a FlagProvider, the error message they get doesn't make it super clear what happened. I'm currently getting:
react.useContext(...) is null. The unleash client defaults the client to null, but none of the hooks actually treat that as an option and assume it cannot be null.I added a small hook that reads the context, and if it is null, throws a better error message. I have not made changes to the
index.tsfile, so this new hook is not exported to consumers. It's internal only to be able to throw a nicer error message.Closes #
Important files
src/useFlagContext.ts
Discussion points