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

Guidelines for choosing between null and undefined with strictNullChecks #9653

Closed
olmobrutall opened this issue Jul 12, 2016 · 20 comments
Closed
Labels
Question An issue which isn't directly actionable in code

Comments

@olmobrutall
Copy link

Whenever you want to make a type optional you have to choose what value to use for the missing values:

  • undefined: Has the benefit that is already there when you don't write anything.
    • lib.d.ts contains 12 references of | undefined.
  • null: Familiarity, better interaction with REST Services (undefined disappears).
    • lib.d.ts contains 473 times of | null.*
  • undefined | null: Is too long, I was surprised that the type expression T? is not introduced as equivalent to T | null | undefined. BTW this example is misleading.

In my opinion for most of the cases the distinction is useless as long as you use non-strict comparison (==. =!) and boolean operators ( && ||).

Unfortunately writing | null | undefined is way too cumbersome, specially if you need have more than one optional thing in a type expression.

This is a real case:

Promise<Array<T | null | undefined> | null | undefined> | null | undefined

Some recomendations?

@mhegazy
Copy link
Contributor

mhegazy commented Jul 12, 2016

Please see #7426 for relevant discussion.

@olmobrutall
Copy link
Author

Ok, thanks!

@mhegazy
Copy link
Contributor

mhegazy commented Jul 12, 2016

One thing to note is that "optional" is really T | undefined ; this is how the JS values will be initialized at runtime. and optional or undefined parameter, property or variable is initialized to undefined, and not to null.

null is a sentinel value that is used to signal the lake of a value. you have to set that up yourself, and it is not done to you by the engine.

Without going into ideological arguments, might be worth watching Douglas Crockford's video when he talks about this in details; the main issue is most programming language use one value to indicate what undefined and null are used for. so in a sense JS has one too many notion of nothing-ness.

The TS compiler internally opted to use only undefined. Part of that is we do not have dependencies on other libraries, so we can be as prescriptive as we want. Some APIs, e.g. DOM, will return T | null consistently, so there is no way around undefined. So i would say it is up to you and your dependencies.

If you want to use both null and undefined i would recommend adding a new type alias: type Maybe<T> = T | null | undefined; and using that everywhere.

@olmobrutall
Copy link
Author

Thanks for the clear TL;DR. I suppose at some point in #7426 the syntax T? was discarded but is hard to find.

Only using undefined looks better in the long run, but I'm scared of APIs, REST services and developers using null from inertia. I'm having a hard time switching var for let if you use C# and TS on a daily basis.

Probably Maybe<T> is the best solution for now.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Jul 12, 2016
@RyanCavanaugh
Copy link
Member

@olmobrutall huge thread but see #7426 (comment)

@olmobrutall
Copy link
Author

There it is :)

@olmobrutall
Copy link
Author

olmobrutall commented Jul 13, 2016

In my quest to use strictNullChecks I'm finding some things that could be interesting. I'm going to write them all here but feel free to ask me to move to a different issue if the thing gets complicated:

  • React render() method throws an error if undefined is returned, but he is happy with null. This makes a nasty exception for my "replace all null by undefined" strategy... if you update render() in react.d.ts it becomes a compile time error, so it's OK.
  • Speaking of .d.ts, is there any reference declaration type that is updated for strictNullChecks already?
  • In a world with strictNullChecks, is there any value on making non-nullable indexers, like this:
let error: { [member: string]: string }

there is always going to be some key for witch you won't have a value...

@RyanCavanaugh
Copy link
Member

I'm not aware of any reference .d.ts files yet.

We decided to make indexers not introduce undefined into the type because there's so much code that indexes from a set of known-good keys into an indexed object in a way where we can't detect that the key is correct (for example, enumerating based on Object.keys(x)). If you want to be super-correct you could declare it : string | undefined but I suspect it will do more annoyance than good.

@olmobrutall
Copy link
Author

olmobrutall commented Jul 14, 2016

We decided to make indexers not introduce undefined into the type because there's so much code that indexes from a set of known-good keys into an indexed object in a way where we can't detect that the key is correct (for example, enumerating based on Object.keys(x)).

Is there an issue where this was discussed? I think this is not the right decision:

While using strictNullChecks sometimes I need to mark something as nullable just for a 5% of rare cases, while a very typical use case for indexers are caches, where you want the compiler to warn you of you didn't check for undefined.

What make sense for me is that if the return type of the indexer is T means that the values that you can encounter when the key is there will be T, or what is the same, the return type of Object.values will be T[].

But when you acces the object using the indexer, undefined should be included automatically, you can always remove it with !:

Object.keys(errors).map(k=>errors[k]!);

Is there any argument agains it?

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 14, 2016

The expression

Object.keys(errors).map(k => errors[k])

has type any[]. Since undefined is a subtype of any, undefined is included.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 14, 2016

If indexers contained undefined every array access a[i] would be T | undefined. This is very annoying and would be borderline unusable.

@RyanCavanaugh
Copy link
Member

you can always remove it with !:

If you were indexing by the wrong key, adding the ! doesn't make you index by the right key. If you indexing by the right key, adding the ! is just an annoyance. All that happens in practice is that the syntax for indexing is x[k]! instead of x[k] with no added type safety.

@olmobrutall
Copy link
Author

olmobrutall commented Jul 15, 2016

If you were indexing by the wrong key, adding the ! doesn't make you index by the right key. If you indexing by the right key, adding the ! is just an annoyance. All that happens in practice is that the syntax for indexing is x[k]! instead of x[k] with no added type safety.

Well, ! is just a shortcut for a casting to the non-nullable version of the type.

As any casting the compiler asks the developer to pay special attention to a potentially unsafe action, and in exchange the compiler trust the developer criteria.

Using an indexer is unsafe, because independently of the declared return type, potentially you'll get back some undefined values.

Example:

const cache :{ [v: number] : number} = {};
function fib(v : number){
    if(v <= 1) return v;

    const c = cache[v];
    if(c != undefined)
        return c;

    return cache[v] = fib(v-1) + fib(v-2)
}

I think it's obvious that the type of c should be number | undefined from a formal point of view.

Also the undefined gets introduced in the indexer action, not the data structure itself, because Object.values(cache) should be of type number[].

Something different is that this would be annoying in many other practical examples, but I suppose I have now a thicker skin after my quest to enable strictNullChecks ((signumsoftware/framework@88ad60c), (signumsoftware/framework@b480420))

Just after turning the flag on, I've found 800 errors in a ~50 files project, of witch I've solved until now 600 (two evenings).

The investment necessary to introduce the flag is quite high. I was expecting something like 4 compile-time errors for each potential run-time bug fund.... but it's more like 50, and what is worst, the way to solve most of them is by adding | undefined and ! everywhere, making the code harder to read.

Here are some things that are creating me pain :

  • Deciding between null and undefined:. Since you took the safe/strict choice of not deciding what T? means, now every project has to do it.

Solution: I'm trying to follow Typescript and replace all our null by undefined but I've found some problems.

  • React render function accepts null but not undefined as a return value. It's happy about undefined inside the React.Elements however. Quite odd...

Solution: By changing the .d.ts you get nice compile-time errors, but you'll have to fix them manually.

Solution: I'm seriously thinking in throwing setState out the window and using forceUpdate everywhere to be able to define the state more accurately.

  • Receiving and sending Json objects: null and undefined are serialized differently,
JSON.stringify({}) //"{}"
JSON.stringify({ name: undefined }) //"{}"
JSON.stringify({ name: null }) //"{"name":null}"

For me this has important consequences, since in one case the property in the server side will be ignored (undefined) while in others will be set to null.

Solution: This forces me to treat null and undefined differently again and decorate the types accordingly. Fortunately they are auto-generated.

  • Promises: When using then and returning undefined and a Promise<T> he is not able to infer that the return type is Promise<T | undefined> (signumsoftware/framework@88ad60c), this is quite annoying for UI modal workflows that can be cancelled at any point.

Solution: I hope async/await will solve this.

Sorry for my long rant, I'm a very enthusiastic Typescript and C# developer and I had lots of hopes in non-nullable references. I also develop the framework in my free time. If I consider this a tough pill to swallow, I can not imagine in a more enterprise environment...

What's even worst, I think there is not too much that Typescript or any language that deals with the real world can do to solve the problem once and for all... looks to me that there are lots of variables that are nullable only in 5% of the cases, so the compiler gets a lot on the way to provide little help.

@olmobrutall
Copy link
Author

After 2 months of working with not-nullable types I want to re-evaluate my opinion.

The transition was a hard, forcing you to re-architect parts of the application and be more careful about nullability (expected) and the subtle differences between null and undefined (unexpected) but I'm very satisfied with the end result.

Thanks for the great work!

PD: Maybe you can forward this to C# team to give them some courage for the next version ;P

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Sep 16, 2016

@olmobrutall the C# team is definitely considering this. There is an extensive proposal and a lot of discussion around the issue of non-nullable types. Check it out dotnet/roslyn#5032

@olmobrutall
Copy link
Author

I know, I've been very active in this thread and in the previous one in Codeplex, but it has been delayed for C# 8 :S

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Mar 25, 2017

One thing to note is that "optional" is really T | undefined ;

For future readers, this isn't quite true. You can't define an interface with a property T | undefined if you want that property optional. Instead, you must use the ?: syntax to have an optional interface property: interface I { name?: T } Using interface { name: T | undefined } will force you to explicitly assign undefined to property name, but it will not allow you to leave it out. Unfortunately, this means that there are really 3 types of "no value" in TypeScript (?:, undefined, null).

@aluanhaddad
Copy link
Contributor

@MicahZoltu there is a syntactic sugar for that

interface I {
  name?: Type;
}

It is a little inconsistent, probably because that notation predates --strictNullChecks and is still very significant even in its absence, but it is very useful.
The tricky part is that if your code depends on the key name being returned by Object.keys then it will behave differently if the property is explicitly initialized.
However there are other areas where we can encounter the same situation. For example classes and object literals use analogous syntax for defining members but with wildly differing implications. It is best not to rely on this difference at all.

@MicahZoltu
Copy link
Contributor

@aluanhaddad Sorry I wasn't more clear, that is what I meant . Will edit my comment.

@aluanhaddad
Copy link
Contributor

I see now what you meant. My take on it is that, even though what you say is technically correct, name?: Type has become so idiomatic as meaning optional property or optional parameter (type of symbol is Type | undefined) that it doesn't matter so much. Basically I am suggesting that Type | undefined as an explicit annotation should almost never be used.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

5 participants