[core] Mark context value as nullable for optional providers #20278
Conversation
`useFormControl` will return `undefined` if there is no `FormControl` provider above in the React tree. `FormControlContext` doesn't provide a default value (maybe it should? See DefinitelyTyped/DefinitelyTyped#24509 (comment)).
Details of bundle changes.Comparing: 6d62842...afcd7b3 Details of page changes
|
|
TL;DR: TypeScripts non-null assertions ( If this would be entirely static I wouldn't mind having a sensible default value. But we have things like So far crashing with |
eps1lon
changed the title
|
Personally I would prefer a default value that throws when calling onBlur, onFocus. Any thoughts @oliviertassinari? |
|
I think that we face the same problem with I find It also makes it easier to console.log the presence of a parent provider. |
oliviertassinari
changed the title
|
Isn't that an argument for the a default value? Since we guard against it anyway? |
|
@eps1lon I didn't understand the why in DefinitelyTyped/DefinitelyTyped#24509 (comment). |
To avoid branching. But back to the original point: Isn't your example an argument for a default value? |
|
Looking at the logic of InputBase, it seems that we do want this branching. We implement different behavior depending on the presence or not of the parent element. Providing a default value would make branching harder (need an extra key). |
|
Ok so your first example did not support your argument in any way. The provider is optional. Nothing more to that. |
|
@eps1lon From what I understand, it boils down to, what's simpler/more intuitive between: const context = useContext();
// 1. with a default value. Because we need to branch based on the presence of the parent element,
// we need a way to know a new context was injected, introduce the `provided` key.
if (context.provided) {
vs
// 2. `undefined` is the default value
if (context) {I would say, 2. But I don't see a strong difference. Happy to go with 1. |
|
If our method/hook/anything returns Typescript — is more than just autocomplete tool, it is type system (not perfect) but type-system. It must guard you from runtime errors. |
|
I didn't argue with that @oliviertassinari. Your first example just didn't showcase any code that wouldn't be better off with a default value. @dmtrKovalenko That is one mentality for TypeScript, yes. It isn't that black or white though. TypeScript itself wants to be "sensible" about this by "[striking] a balance between correctness and productivity.". A good example for this are index signatures of arrays. They will return a non-nullable type even though they could be undefined. This was deliberate because they think it hurts more JS devs than it helps. Applied to our case: If the Provider is required anyway then it doesn't help much to make the value nullable. We do this for the ThemeProvider for example. I think required + no-default value make a good case for not returning nullables. You wouldn't want to guard against undefined for every usage of useTheme leading to non-null assertions at every callsite making the nullable type useless. |
eps1lon
changed the title
@eps1lon Agree, the Radio wasn't the best example I could have link to. I should have used the InputBase.
Oh wow, super interesting. In our case, does it apply? We could potentially say that 99% of the cases, people will use the hook within an context provider. Maybe what we had before was already the best tradeoff? Not sure.
💯 agree. We provide this default value: |
@dmtrKovalenko I don't think that it matches with the spirit of all the tradeoffs made by TypeScript, as highlighted by Sebastian. They can be exceptions. It resonates with this past discussion mui/material-ui-pickers#1575. |
Yeah this is a nuanced issue. It's not just as easy as "returns undefined -> must be typed as such".
That's a good point. Depending on the tree it might be hard to spot which component is the child of a provider and which isn't. From my perspective hitting "cannot read x of undefined" when accessing context values is a clear sign that the provider is missing. But that is when using react every day and seeing these issues often. Might be that it isn't that obvious for others. I just think that in most cases people will go for But at least So I think
1 Context that you have rarely more than once in your tree and then at the top of your tree. |
|
Yeah, definitely I am fan of type soundness, but in this particular case I think that was right because it can confuse somehow with an error message |
oliviertassinari
added
component: radio
oliviertassinari
added
the
component: text field
useFormControlwill returnundefinedif there is noFormControlprovider above in the React tree.FormControlContextdoesn't currently provide a default value. Perhaps a default value should be provided? See DefinitelyTyped/DefinitelyTyped#24509 (comment).This could be considered a breaking change for folks that are currently using this hook, as they will now need to account for the
undefinedcase. I'd be happier providing a default value toFormControlContext, but wanted to run it by you first.