-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Optional chaining of unknown should be unknown #37700
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
Comments
This is as safe as allowing property access on And if this would be allowed, why should it be restricted to the optional chaining syntax? function f(value: unknown): unknown {
if (value != null) return value.someProp;
return undefined;
} I believe it is semantically equivalent. |
I think the real weirdness is that it'd break the operational directionality of This would lead to some subtle breakage in the future, e.g. let's say you wrote function fn(x: unknown) {
if ('y' in x) {
console.log(x?.z);
}
} The proposal is to make this code OK, but then in the future it'd be a breaking change for us to narrow |
Should use |
Hi Ryan thank you for your excellent example. I agree that narrowing should only ever increase the set of things that are allowed to do. But I can think of a counterexample involving the use of contravariance.
As you see class narrowing does not always increase the set of actions that are allowed to do. I believe this Beside theoretical issue, in practice use, people will just be fine to fix the optional chaining of a variable when they decide to narrow the type. Comparing to using Regarding the breaking change concern, if this proposal is published along with the type narrowing of |
Isn't this also true for
I think you meant
It does increase it from |
For a concrete example, the famous react-router's Now referring to the state is pretty common, here is an example similar to the official documentation: function LoginPage() {
const location = useLocation();
const { from } = location.state || { from: "/" } };
....
}; Unpacking const { from } = typeof state?.from === "string" ? state : { from: "/" }` Or maybe there is a way to handle this simply without optional chaining on unknown? |
Considering that this is possible with |
Why should two types designed for completely opposite purposes ( |
@RyanCavanaugh becauae the entire purpose of In other words, the only difference between |
@ljharb I can't tell what you're trying to say with that. If you desugared function f(value: unknown): unknown {
return value?.foo;
} into function f(value: unknown): unknown {
return value == null ? undefined : value.foo;
} TypeScript would error on that, same as we would error on It seems like "New syntax constructs should behave like their desugarings" would be an uncontroversial interpretation but that's not what I'm hearing here. |
@RyanCavanaugh |
The reason the parallel is important is because as already mentioned, with |
Nit: Ergonomics isn't necessarily more important than soundness, it's just that it's allowed to be considered (in contrast to languages that would never make a soundness/ergonomics trade-off). |
What's the rule that says it's OK for |
@RyanCavanaugh that's true, but since TS is incapable of properly handling things with a [[Prototype]] of |
Even better, that means since |
My information is, that it works like this: Using
I don't know of any way to make |
Every time i need to handle an error I am landing back at this issue. Considering that the usage of const handleError = (error: unknown): void => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const errorType: unknown = (error as any)?.graphQLErrors?.[0]?.errorType;
if (errorType === 'MyErrorType') {
handleMyError();
return;
}
throw error;
};
try {
makeApolloGraphqlClientQuery();
} catch (error) {
handleError(error);
} The apollo graphql client puts the |
you should be able to rewrite that line like that in order to not disable eslint
Thanks @falsandtru for help in figuring this out |
This does not work for me, I get the error "Object is of type 'unknown'.(2571)". |
Got it working on Quokka but apparently it won't compile Sorry for misleading information... |
Perhaps if we looked at it from a different perspective: Basically, Or another perspective: the desugaring is in JS semantics, not TS semantics. We expect I don't want to change a spec post-hoc, I'm just trying to explain where I come from re intuition of semantics of this feature. Curious to hear your thoughts. |
I mentioned this in a related thread, but I think just coming at this from a set theory/mathematical perspective should carry some weight. No matter how we want to "think" about Granted, I'm playing a little loose with the math... we'd have to consider the operator acting on an infinite union of sets and whether that's the infinite union of the operator acting on those sets... but I think it's safe to say that when you're dealing with the range being all types, you can at least intuitively conclude that the range as being all types. But hey, if you can find an example of a type that optional chaining can't map to, then you win. Yes, I get that Personally, I feel like the set theoretical interpretation of the type system is foundational. It's really helped me to explain to new devs how Likewise, keeping this in mind we can see that using And although it's been mentioned multiple times, I just want to quickly reiterate how this would improve type safety as the compiler would then force us to use optional chaining, as opposed to when |
I think you can't mention this often enough, as it outweighs all the counter-arguments in my opinion... 😅 |
Just chiming in to say that @WIStudent's prediction has come true. I am one of the people who has updated their TypeScript version and now wishes they could use optional chaining in catch clauses. |
This is another place where "technically works" doesn't mesh well with our philosophy of preventing real errors that you would write. Allowing One anecdote I'll provide here - a while back, there was a team decision to allow property accesses on any object with a string index signature; after all, any property access ( |
What bugs did that introduce? |
Basically the effort of turning on that flag was too high to casually completely adopt it in our testing suite - that's part of the problem. As you depend on this behavior accidentally, you can't easily remove it in a refactor - but you can see a bit of when I started it here. #41784 I found interfaces where properties that didn't exist were being accessed, or were declared as a |
Thanks for the explanation. Could I ask what exactly you mean? Why are the proposed semantics for propagating Do you know of a real error that people would write with this? I hate to ask for an example of a theoretical problem, but on the other hand, I'm literally writing real errors now, without this feature, by casting to How do you propose writing a safe JSON decoding routine? E.g., access Maybe there's a common idiom I'm missing which makes |
@DanielRosenwasser so am i understanding properly that then, as well as now, you’re thinking that the migration pain isn’t worth the bugs it will catch, but that it should have been this way from the start? |
Based off the mention in #46314, I would guess that the potential bug would in the class of misspellings of properties i.e. Counter-argument: In my opinion, pretty much any other argument can be boiled down to a misunderstanding of the nature of For example, from #46314
No, actually, it's not... just take it back to the math. To say As far as I can think of, To sum up, I really want to press the point that rejecting this proposal is not based on the mathematics of the type system. This is choosing an idiom for the language... which I'm actually okay with. I'm a big fan of Go largely because of how idiomatic it is. |
What I do, is upcast to a conservative version of the type I expect. In your JSON decoding example, I would do: type Unreliable<T> = { [P in keyof T]?: Unreliable<T[P]> } | undefined;
const decodedData = JSON.parse(data) as Unreliable<{ a: { b: { c: number }}}>;
decodedData?.a?.b?.c; // Type is number | undefined The compiler enforces the use of the If you would like the compiler to enforce the use of the JSON.parse(data) as Unreliable<{ a: { b: { c: unknown }}}>; It's a bunch of extra characters, but comes with the advantage of only allowing access to properties you declared as potentially existing. (No |
This will allow people to actually use |
Thanks for your reply, I just wanted to focus on the The comment you linked to (#46314) says short-circuiting would create the following risk: if (a?.foo) {
a?.boo.yadda(); // oops
} Maybe someone has a more realistic example, because this is precisely the kind of code you use optional chaining to avoid. You'd write: a?.foo.yadda(); that's the point of null coalescing member access, as I understand it. But how would you do this currently? Cast to I guess my question is: Is there a solution to handling |
I was just saying that's the best argument against, not that I was a good argument against lol. I'm personally advocating for optional chaining to be allowed on unknown for mathematical reasons. Set theory, the basis of the type system itself, and the nature of the The case of typos is a valid concern, but it becomes a question of what's more important: a logically consistent type system or avoiding a class of run-time errors. The type system is supposed to help avoid the latter, but does that make it less important? I don't think so, but others clearly disagree. But even the argument about avoidance of runtime errors has to be taken as what actually is going to prevent more run time errors, not what might. That ignores the human element. If more of the community stops casting to any because of optional chaining on unknown, that's safer code. |
For those interested, you can somewhat emulate this behaviour like this: type SafeAny = {
[key: PropertyKey]: SafeAny,
} | undefined | null;
const x = null as SafeAny;
console.log(x?.children?.[0]?.age); // This is allowed
console.log(x?.children[0]?.age); // This is an error The caveats here are:
|
this is how I manage this problem (feel free to use it or improve it, for now it only works for one level deep properties) : export const getUnknownProperty = <T extends Typeof>(
unknown: unknown,
propertyKey: PropertyKey,
typeofProperty: T
): TypeSafePropertyValue<T> | undefined => {
if (!unknown) return undefined;
if (typeof unknown !== "object") return undefined;
const unknownAsRecord = unknown as Record<PropertyKey, unknown>;
if (typeof unknownAsRecord?.[propertyKey] === typeofProperty) {
return unknownAsRecord?.[propertyKey] as TypeSafePropertyValue<T>;
}
return undefined;
};
type TypeSafePropertyValue<T extends Typeof> = T extends "bigint"
? number
: T extends "boolean"
? boolean
: T extends "function"
? Function
: T extends "number"
? number
: T extends "object"
? object | null
: T extends "string"
? string
: T extends "symbol"
? symbol
: undefined;
type Typeof =
| "bigint"
| "boolean"
| "function"
| "number"
| "object"
| "string"
| "symbol"
| "undefined"; |
const c = decodedData?.a?.b?.c // Type is number | undefined
if(c){
c // number , but is it really a number type in js runtime ?
}
type Unreliable<T> = T extends object
? {
[key in keyof T]-?: Unreliable<T[key]>
}
: unknown |
Thanks for this @Alexandre-Fernandez! I'm using it with a little tweak: instead of : T extends "object"
? object | null I use: : T extends "object"
? { [key: string | number]: unknown } | null which makes the following safer: const prop = getPropFromUnknownType(someUnknown, 'prop', 'object')
const subProp = prop?.['subProp'] When using |
function isTom(cat: unknown) {
const firstName = cat?.name?.first
if(typeof firstName === 'string' && firstName === 'Tom') return 'Yes'
else return 'No'
}
isTom('Jerry') // No
isTom(null) // No
isTom(123) // No Code above will cause a compiling error in TS. |
Disallowing optional chaining on try {
// some code
} catch (err: unknown) {
console.log(err?.message); // Not allowed
} const response: unknown = await getResponse();
const isOk = response?.status === 'ok'; // Not allowed
if (isOk) ... const data: unknown = await getData();
const nestedValue = data?.some?.nested?.value; // Not allowed
if (isWhatINeed(nestedValue)) ... Is it possible to reconsider this issue? |
Uh oh!
There was an error while loading. Please reload this page.
Search Terms
Optional chaining, unknown
Suggestion
Currently it is forbidden to use optional chaining on
unknown
. But for all possible types in JavaScript, optional chaining will returnundefined
if it does not apply or cannot find the field. So we should safely regard optional chaining ofunknown
type to beunknown
as well.This is the verification of optional chaining on all possible types in JavaScript:
Use Cases
This is to simplify type checking on nested object with unknown type. For example, we should be able to do something like this:
Currently we will need to cast explicitly on
someUnknown
,someUnknown.fieldA
,someUnknown.fieldA.fieldB
andsomeUnknown.fieldA.fieldB.fieldC
. People will find this is unnecessary verbose and may just fallback to useany
.Examples
As above
Checklist
My suggestion meets these guidelines:
The text was updated successfully, but these errors were encountered: