TypeScript: Context.Provider value should not be assignable to undefined in the default case

[deluksic]

Summary

Context providers should not accept undefined by default. Example of previously correct code we'd like to avoid:

const MyContext = createContext<string>()
const someValue: string | undefined

<MyContext.Provider value={someValue}>
  ...
</MyContext>

Assumption is that if you really wanted to allow providing undefined you would have written:

const MyContext = createContext<string | undefined>()

This does not change the fact that useContext returns undefined if no default was provided:

const MyContext = createContext("hello")
const MyContextWithoutDefault = createContext<string>()
const MyContextExplicit = createContext<string | undefined>("string")

const ctx: string = useContext(MyContext)
const ctx: string | undefined = useContext(MyContextWithoutDefault)
const ctx: string | undefined = useContext(MyContextExplicit)

When combining Resources with Context, it is very easy to forget to test if a given resource is available before assigning it to the provider value.

How did you test this change?

I wrote an additional test to test the usage patterns.
This is type-level only change.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 91d3b18

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[otonashixav]

I don't believe this requires adding a generic; regardless it will be a breaking (type) change. You'd need a second Context type for the defaultless overload, with useContext accepting either. There are a few different ways to change useContext for this purpose but overloading it probably makes the most sense. The second overload should accept both while the first accepts the narrower context.

[deluksic]

Yes, I considered this as well, just thought this approach was a bit simpler. I'm not super worried about this detail, as long as the user-facing API is improved. Should I create another PR with your suggestion and we compare?

regardless it will be a breaking (type) change.

Yeah, but hopefully people have not relied on this, and would actually like to see the error otherwise (given that it appears only when strict enabled). For me it was surprising that doing

<Context.Provider value={undefined}>

actually reverts to the default value and doesn't literally set undefined. This could be reflected in the types, but I wonder why it is like this in the first place.