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

Abandon null in favor of undefined #42713

Closed
NicBright opened this issue Jun 30, 2021 · 9 comments
Closed

Abandon null in favor of undefined #42713

NicBright opened this issue Jun 30, 2021 · 9 comments
Assignees
Labels
area: core Issues related to the framework runtime cross-cutting: types feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Milestone

Comments

@NicBright
Copy link

NicBright commented Jun 30, 2021

Feature Request

Relevant Package

  • @angular/forms
  • Possibly any other package where null is used over undefined

Description

At the moment of writing this (June 2021), in the Angular code base, there seems to be no clear guideline for when to use null and when to use undefined. A brief search shows that (in the Angular code base starting from ./packages) assignment to null ( = null, notice the leading space to avoid matching === null) occurs 1448 times, and assignment to undefined ( = undefined) can be found 1905 times. While it is understandable to accept and properly handle passed null values from the point of view of public api (because Angular is a framework after all and users should be able to use whatever value they like), it is not understandable (to me), why null is used internally at all. This is problematic, because it makes it very easy for the null value to bleed through Angular's public api (which it shouldn't!).

(Imho) Nowadays, the type null just shouldn't be used in modern TypeScript code any more. The reasons for that are simple:

  • TypeScript offers handy shortcuts to "add" the undefined type to a type definition via ? character: e.g. myClassPropertyOrMethodParam?: number. Using null instead just feels awkward (e.g. myClassPropertyOrMethodParam: number | null) and makes the code less readable. In that case you will end sooner or later adding undefined, too (e.g. myClassPropertyOrMethodParam?: number | null, some people even prefer to add undefined explicitly in that case to "improve readability" myClassPropertyOrMethodParam: number | null | undefined).
  • undefined is the natural default type of anything that has not been initialized.
  • last but not least, the classic: typeof null === 'object' // sadly true.

Consider Brendan Eich's two-word suggestion: "Use undefined"

Example where null bleeds through Angular API

There's one module where it particularly bothers me, that Angular uses null, because null bleeds through. What I mean by "bleeding through" is, that the null type is exposed via API that is consumed by Angular users:

The @angular/forms package assigns null as the default value to a form control! See here and here!

Why is this a problem?

This is a serious problem (imho)! What Angular is doing here is very offensive! It is forcing its users (or say at least it's tricking its users below senior level, which could well be >80%) to include the null type in their own API / interfaces, because it's just possible that a form control is in that state! This can make code more complicated than it has to be. It actively hinders people from trying to avoid null inside their own code base. For example what I can see regularly in our code base are things like this:

export interface SomeType {
  someProperty: number;
  someOptionalProperty?: string | null
}

When I ask them why they've included null in their type, what I usually hear as an explanation is "because otherwise it would not have been compatible with Angular Forms API".

To be clear: while adding null to someOptionalProperty is no harm, it is just plain unnecessary here! While it looks like no big deal, it really is! Like cancer, adding null to your types can spread easily to other places in your code base. A method that in the past was properly typed with optional parameters (param?: SomeType) will soon be widened to accept null as well (param?: SomeType | null). This is, because in a lot of projects, developers just need to get work done, so it's understandable why they're saying "it's easier than fixing the null type in my interface right now" and "I'm in a hurry and need to call that method now!".

Where null can't be avoided

You can't always avoid having null values. This is, because JSON doesn't know the type undefined, it only knows null. However, in my eyes, this is not an excuse to carry the null type around all over the code base. I'm not saying to not accept null values coming in from JSON. All I'm saying is to stop using null actively.

How does a code base not using null at all look like?

When following a few coding guidelines, null can always be implicitly treated as undefined and thus there's no need to add it to any types / interfaces at all.

Here are the guidelines that make it possible:

  • avoid direct undefined type guards (e.g. if (value === undefined) { ... }.
  • Instead, use indirect type guards (aka truthiness checks) e.g. if (value) { ... }
  • Whenever 0 or empty strings are meaningful, use either
    • an explicit helper method like Lodash's isNil
    • or include the meaningful value in the comparison (e.g. if (!value && value !== 0) { ... })
  • Consider using a lint rule that disallows the usage of null

Describe the solution you'd like to see in Angular

  • all public api should continue to accept null (ideally only as part of any type) and handle it properly
  • all public api should stop returning null values
  • all public api that used to return null values should return undefined instead of null

Consequences for Angular

I know that this is a huge change and a breaking one as well. A rather long deprecation period would therefore be needed for user adoption.

Describe alternatives you've considered

As it is a matter of (admittedly very debatable) taste, there will surely exist a multitude of alternatives and other opinions. I would like to not open up discussion here on how null is useful to somebody in some cases. Such a discussion can be found here ... Instead, please just up/downvote this feature request if you agree/disagree. Thanks a lot!

Best Regards, Nic

@jessicajaniuk jessicajaniuk added the area: core Issues related to the framework runtime label Jun 30, 2021
@ngbot ngbot bot added this to the needsTriage milestone Jun 30, 2021
@evgeniyefimov
Copy link

I 100% agree that null should be abandoned, but it has already been discussed here #16982 (comment) I think nothing has changed since then.

@NicBright
Copy link
Author

I 100% agree that null should be abandoned, but it has already been discussed here #16982 (comment) I think nothing has changed since then.

Thanks, @evgeniyefimov, for pointing me to that discussion which I haven't been aware of. As @mhevery pointed out, it's a huge breaking change to migrate from null to undefind. I'm aware of that fact. However, in theory, it should be possible to write an automatic code converter that could help with migration (via ng update). In the end we should just "Do the right thing" (which imho is to switch from null to undefined).

@lazarljubenovic
Copy link
Contributor

Funny; I'd be using only null instead of only undefined if I had to strictly choose.

Firstly, while I agree that there should be a convention, I don't think all instances of either can be replaced with one of those. As you said,

undefined is the natural default type of anything that has not been initialized

which, to me, means that undefined would be the value of something that I've never touched; but if I explicitly define something, then tha would be initialize with null. It wouldn't make sense to me to "initialize something with the value that is used when something is not initialized".

For example, the way I see it, Map#get should return undefined when you try to access a key that hasn't been defined (yet). It doesn't make sense to imagine a code that firstly sets all possible keys to the value of null (there can be infinitely many if you ignore practical hardware constraints). However, when there's a finite, pre-defined set of possible keys, null can be the "default" value which means "nothing"; e.g. Element#onclick is null by default because nothing will happen when an element is clicked, but it's clear that it's possible for something to happen (compare with asking for Element#onsing, which returns undefined and points to a fact that you might've made some mistake, since it doesn't make sense to listen to a nonsense event).

For example, I agree that async pipe should return undefined before it sees a first subscription, precisely because I expect to use null myself for something meaningful. undefined could produce a loading indicator in the view, while null could tell me that some value is explicitly "nothing".

Secondly, you suggest some strategies which I've seen people avoid with time, including myself.

Instead, use indirect type guards (aka truthiness checks) e.g. if (value) { ... }

This is a footgun whenever a falsy value should not be treated as null/undefined. For example, 0 can have a different meaning than undefined (e.g. delete (count?: number) might delete all elements when called as delete(), but obviously delete no elements when called as delete(0)).

To avoid thinking about null and undefined (since you don't care in a lot of cases), you can use if (x != null).

if (x !== undefined) too strict
if (x) too lenient
if (x != null) just right

It's also easier to understand the intent behind x != null over !x. ! should be used only with booleans, since that's the only place where its meaning is clear.

I'm quite confused as your list then jumps to this example"

if (!value && value !== 0) { ... }

without even mentioning a simple value != null. Is the == null something you've been purposely avoiding? If so, can you explain why?

@NicBright
Copy link
Author

NicBright commented Jul 28, 2021

It wouldn't make sense to me to "initialize something with the value that is used when something is not initialized".

In all my professional career I never had the need to distinguish between something "initialized with null" and something that has not been initialized. For me, this is semantically the same thing. Even more so, because when you receive null from outside your application, you really never can be sure about the intended semantic there: did the developers responsible for managing that data (technically) thought of null to mean "initialized explicitly and intentionally as null", or is it just null because it's the undefined of the sql database world?

Regardless, at runtime, it just doesn't matter if something "is null" or "is undefined". It's both semantically the same thing: i.e. "nothing". For example, you have been mentioning Element#onclick vs. Element#onsing -- for me the difference is irrelevant: if there's nothing there, undefined is just the right value; while the DOM specification decided to use null for a missing onclick it better had chosen undefined instead. Because in practice it just doesn't matter if there COULD be a onclick. If it's not there it's just not there ... only if you really need the assurance at runtime that you really didn't accidentally add a typo in your onclick (checking with null, compared to onsing returning undefined) makes your argument a little bit valid, but that's a matter of taste, for my part, I don't need that because it's a very weak argument and the benefit is almost zero to me, if not negative, because it adds unnecessary extra complexity to my code.

Concerning your "async" example, I would rather recommend to use an explicit value for indication of "LOADING" state, rather than overloading "undefined" with that semantic.

Map#get should return undefined when you try to access a key that hasn't been defined (yet).

This is exactly how Map#get works! But rather than needlessly "initializing" values with null I suggest you use Map#has instead to see if a value has been defined or not.

This is a footgun whenever a falsy value should not be treated as null/undefined. For example, 0

That's exactly why I've added the third rule:

Whenever 0 or empty strings are meaningful, use either
an explicit helper method like Lodash's isNil
or include the meaningful value in the comparison (e.g. if (!value && value !== 0) { ... })

Coming to your last argument:

To avoid thinking about null and undefined (since you don't care in a lot of cases), you can use if (x != null).
...
Is the == null something you've been purposely avoiding? If so, can you explain why?

Yes, I've been actively avoiding loose equality comparison with null, because it requires you to use, well ... null which (for me) is the general idea: null is so utterly useless to me, that I don't want to read it in my code at all, if not required for compatibility reasons (hopefully limited to a very few places where interoperability with data coming from outside / third party libs is required). Adding to that, I think it's useful, in general, to avoid loose equality comparisons and use strict equality comparisons instead, because for most people, implicit type conversion is a black box resulting in their code becoming kind of a random surprise egg -- working in most happy cases and sometimes just not working quite as expected. It's far easier to learn the rule for "truthiness/falsiness" (i.e. conversion from anything -> boolean) than for conversion from anything -> anthing.

@Den-dp
Copy link

Den-dp commented Sep 22, 2021

I also found that since @Optional() dependency is null, it is impossible to make use of the default parameters feature:

  constructor(@Optional() @Inject(API_ENDPOINT_URL) private apiUrl= '/api') {
    console.log(apiUrl); // null
  }

Default parameters only work with undefined.

@jessicajaniuk jessicajaniuk added cross-cutting: types feature Issue that requests a new feature feature: under consideration Feature request for which voting has completed and the request is now under consideration labels Dec 13, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 13, 2021
@jessicajaniuk jessicajaniuk added this to Inbox in Feature Requests via automation Dec 13, 2021
@pkozlowski-opensource
Copy link
Member

The issue of @Optional() with DI is tracked already in #25395

@pkozlowski-opensource
Copy link
Member

While I appreciate the sentiment of having strict guidelines and usage patterns for undefined vs. null - those types exist in both JS and TS and there are legitimate cases where we want to distinguish between "not initialised yet" (undefined) and "initialised with no value" (null). We are using this pattern quite a bit in the fwk codebase.

@pkozlowski-opensource pkozlowski-opensource closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2022
Feature Requests automation moved this from Close with Followup to Closed Jun 30, 2022
@NicBright
Copy link
Author

NicBright commented Jul 12, 2022

there are legitimate cases where we want to distinguish between "not initialised yet" (undefined) and "initialised with no value" (null).

@pkozlowski-opensource can you point me to a few examples where this distinction is relevant? I'm asking because I'm really curious and couldn't find such a use case until now :-)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime cross-cutting: types feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Projects
Development

No branches or pull requests

6 participants