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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

deluksic
Copy link

@deluksic deluksic commented Nov 17, 2023

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.

Copy link

changeset-bot bot commented Nov 17, 2023

⚠️ 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

@deluksic deluksic changed the title Typescript: Context.Provider value should not be assignable to undefined in the default case TypeScript: Context.Provider value should not be assignable to undefined in the default case Nov 17, 2023
@deluksic deluksic marked this pull request as ready for review November 19, 2023 09:30
@deluksic deluksic force-pushed the deluksic/enforce-context-provider-types branch from bc7714a to af13719 Compare November 19, 2023 09:50
@ryansolid ryansolid added the typescript relating to typescript or types label Nov 21, 2023
@otonashixav
Copy link
Contributor

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
Copy link
Author

deluksic commented Dec 4, 2023

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.

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

Successfully merging this pull request may close these issues.

3 participants