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

Elvis operator in expressions: ?. #791

Closed
mhevery opened this issue Feb 25, 2015 · 45 comments · Fixed by #2152
Closed

Elvis operator in expressions: ?. #791

mhevery opened this issue Feb 25, 2015 · 45 comments · Fixed by #2152
Assignees

Comments

@mhevery
Copy link
Contributor

mhevery commented Feb 25, 2015

Currently a.b.c will thrown an error in expression if a is null. To suppress null dereferencing we should support a?.b.c which would short circuit further evaluation if a is null. In this way we could have explicit notation which would show which values could be null.

Syntax to fallow dart proposal: https://github.com/gbracha/nullAwareOperators/blob/master/README.md

@jelbourn
Copy link
Member

jelbourn commented Mar 2, 2015

What are the reasons for not making this the default behavior (similar to angular 1)?

@mhevery
Copy link
Contributor Author

mhevery commented Mar 3, 2015

More expressive. It hints to the reader what may be lazy loaded and what may not.

Example: user?.location.address would say that user is probably async loaded, but once we have a user we must have a location and address.

@jelbourn
Copy link
Member

jelbourn commented Mar 3, 2015

Feels kind of weird to me; it's surprising to have language extensions within the binding expression syntax.

This might also need some more thinking, but I don't see this as being that useful. If angular defaults to gracefully handling null dereferencing in expression then the developer doesn't have to care whether something is loaded lazy or not. The times when people do care is when something needs to be shown in place of the pending data, which will still be handled with an if

@mhevery
Copy link
Contributor Author

mhevery commented Mar 3, 2015

@yjbanov, @vsavkin Please comment. I can be convinced either way.

@pkozlowski-opensource
Copy link
Member

Yeh, I might be used to Angular 1.x ways of doing things too much, but I find forgiving evaluation of NG 1 expressions quite handy. I'm not sure if I like the idea of being forced to mark all the optional parts in the expressions: this does things more obvious but more laborious as well.

@erwinmombay
Copy link

I think the explicitness of ?. is welcome, but i also actually feel that the forgiving nature of angular 1.x bindings to be useful too. I guess my question is if the elvis operator ?. is added will the binding still be forgiving? or will accessing a prop on undefined cause a TypeError (i am guessing handling both might add some overhead)?

@yjbanov
Copy link
Contributor

yjbanov commented Mar 3, 2015

I am in favor of having the safe navigation operator (a.k.a. elvis operator). It exists in C#, Swift, Groovy and others, and will likely be added to Dart. So I don't think there is going to be any confusion around it. It will also allow us to retain the JS and Dart semantics of expressions without this operator. Finally, it will result in smaller and more efficient change detector code, as a.b.c expressions become a.b.c. in the change detector.

EDIT: updated the post with correct references to the prior art.

@jelbourn
Copy link
Member

jelbourn commented Mar 3, 2015

@yjbanov This is different from a null-coalesce operator, though. The null-coalesce operator in C# and Swift is ?? and works like:
someValue = possiblyNullValue ?? defaultValue

This can already be accomplished in JavaScript with the OR operator:
someValue = possiblyNullValue || defaultValue

The question here is about the nullsafe dereference operator ?., which would work like:
someValue = possiblyNullValue?.someProperty
Where the right-hand expression would evaulate to null if possiblyNullValue is null instead of throwing an error. After some searching, Groovy is the only language I found that supports this operator.

Did you have some case or scenario where explicitly using nullsafe dereference would be advantageous to making all dereferences nullsafe by default?

@yjbanov
Copy link
Contributor

yjbanov commented Mar 3, 2015

My bad. I meant safe navigation operator, a.k.a. the elvis. I will edit my original post. The point still stands.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 3, 2015

Advantages of having this operator:

  • It is a syntactic extension and not semantic deviation from the main language (JS or Dart)
  • Especially for Dart, which is getting the operator in the future, it is also consistent with the language
  • It results in more efficient change detector code both in code size and speed
  • It serves as an early warning system. Developers will know where they are not handling missing data more immediately
  • It tells the reader of the template code where missing data is possible

I do realize its disadvantages though:

  • It is different from AngularJS/Dart and so existing developers need to adjust
  • It requires one extra character per property access

I believe the advantages outweigh the disadvantages, so I think we should include it.

@jelbourn
Copy link
Member

jelbourn commented Mar 3, 2015

I see additional disadvantages:

  • The operator is not standard JavaScript and would be confusing to JavaScript developers (I would have been confused if I just saw this in a template).
  • We're already introducing a lot of new syntax constructs; adding more further increases the mental cost of getting started.
  • Deviates from how other template systems out there handle nulls (Polymer, Handlebars)

I also would predict that the majority of developers would want the nullsafe behavior by default, vs. throwing an error.

Perhaps this could be some kind of application-level configuration?

@pkozlowski-opensource
Copy link
Member

I tend to side with @jelbourn - Angular 1 did excellent job at making "the right thing most of the time". Using it felt natural and enjoyable from the start. I feel like introducing ? makes the framework do "the right thing all the times" and makes the internal implementation easier / better but at the expense of developer ergonomics.

I totally agree that having null-safe behaviour by default creates a (small) sharp edge and people might got hurt from time to time (things failing silently), though.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 3, 2015

@jelbourn, all three points are along the lines of "this is different from what people are used to and they will have to adjust". I do not see this as a big problem. It's pretty easy, IMO, to get used to typing ?.. It's a tiny portion of your template code anyway. Also, saying "developers would want the nullsafe behavior by default" is not the same thing as "developers would want . to be nullsafe". There are two ways to dereference things, . and ?.. Neither is more "default" than the other. ?. is not there do discourage you from writing nullsafe expressions, only to be explicit in the syntax to help the readers of your code and to help the tools.

@pkozlowski-opensource, how is ?. hurting developer ergonomics? I think it improves it. It's explicit and very expressive.

In dev mode Angular could point to the exact location in the expression that caused TypeError/NoSuchMethodError, so it's easy to either change . to ?. or to deal with the missing data.

@pkozlowski-opensource
Copy link
Member

how is ?. hurting developer ergonomics?

  • need to think about which parts are optional and which not
  • need to type more characters in proper places

Coming from Angular 1 World I see it as an unnecessary burden, compared with the listed benefits. Then again, maybe I've spent too much time writing 1.x code to be objective here.

@caitp
Copy link
Contributor

caitp commented Mar 3, 2015

so it's basically between either hypothetical performance improvements / simplifed code, or making upgrade path easier.

Would the change detector code be just as fast/small if every field in an expression was ?-optional? If not, then how would it be any better than the "easier upgrade path" variation? Is there evidence that the early warnings developers get with it are actually useful? Or is the use mostly just "oops you forgot to add this extra character to this field in some expression somewhere in your project, we aren't sure where, or if it's even your own project or a dependency"

  1. How would you make the early errors useful?
  2. Are the lack of errors (when referencing properties of undefined values in expressions) in 1.x/dart something people actually complain about?
  3. Are the perf improvements hypothetical, or actual?
  4. Is the code actually simpler? somewhere along the way you have to distinguish between optional fields vs non-optional, which probably means extra AST nodes, and code visited by "optional field" nodes needs extra logic to handle with the optional-ness anyways, so where is the opportunity for making code smaller and simpler?

@yjbanov
Copy link
Contributor

yjbanov commented Mar 4, 2015

@pkozlowski-opensource, you don't have to think. Just use ?. everywhere, like you used . everywhere in Angular 1. I already admitted the extra character as a disadvantage. However, the explicitness and early warnings are advantages in terms of developer ergonomics. As web app developers we spend a lot more time trying to figure out what to do than typing code. So it seems like a good trade to me.

@caitp, there are two aspects to the upgrade. There's the mental migration from Angular 1 concepts to new Angular 2 concepts, which we discussed in detail above. The second aspect is the actual code migration, but it is pretty straight-forward - you replace every . with ?.. I'm sure it's possible to write a script that does this automatically, but given the vast differences between Angular 1 and Angular 2, it's probably going to be a drop in the sea of other changes you'll have to make.

@pkozlowski-opensource
Copy link
Member

@yjbanov of course "thinking" part is more important / time consuming than "typing" but proposing "use ?. everywhere, like you used ." would make all the expressions look really ugly / less readable (personal opinion). So I would have to still add them selectively (and here comes the thinking part). So maybe an app-level global setting is a way to go?

Anyway, I guess I got my opinion / arguments clear, not sure I could add anything more... Apparently people disagree on what majority developers want, so I can only speak for myself...

@yjbanov
Copy link
Contributor

yjbanov commented Mar 4, 2015

Unfortunately, I don't think a global setting will work as you want to be able to consume third-party components without breaking them. It would also kill the explicitness in the proposed syntax. This is something we have to decide now.

@pkozlowski-opensource
Copy link
Member

Oh, we can't have a switch, you are totally right.

@hworld
Copy link

hworld commented Mar 4, 2015

Any way to have an option on the template annotation to opt-in to it?

  • This would allow template authors to decide if they want the error catching/optimized change detection.
  • It would make migration easier with the ability to "upgrade" templates piece-by-piece.
  • Third-party would be fine since each component/template tells Angular what it wants.
  • CD code shouldn't get more complicated since you already have to have the option for null checks with the ?. option.
  • One could complain that it's annoying to opt-in with each template, but I'd argue it's probably more "annoying" to stop and think about ?. every binding.
  • Doing ?. can also complicate things with templates if you initially have something resolving right away before the binding, but then decide to lazy load it from server in a code iteration.

@mhevery
Copy link
Contributor Author

mhevery commented Mar 4, 2015

  • the ?. will have almost no impact on speed. The reason for that is that we have to read the field (the expensive part) and load it into register. The actual cost of checking for null once data is in register is essentially nothing (single CPU instruction, on modern CPUs with branch prediction this is very cheap). So I would not say that speed is an argument for/or against anything.
  • We can certainly have template level switch so that each component can chose its own semantics.

I think having a template switch to make .? and having explicit .? should make most people happy.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 4, 2015

I think having the template switch is worse than having the operator. We should aim for readability, and this means we should avoid non-locally defined semantics. If I'm reading someone else's code, I shouldn't have to check their options to know if the expressions are null-safe or not (especially when doing code reviews where review tools collapse full file contents). So I think our options are:

  • fully defined . and ?.
  • nullsafe . (i.e. status quo)

I'm still worried about code size. Nullsafe expression a.b.c.d will generate something like:

var tmp;
return ((tmp = a) == null)
  ? null
  : ((tmp = tmp.b) == null)
    ? null
    : ((tmp = tmp.c) == null)
      ? null
      : tmp.d

Maybe there's a better way. In JS you can take advantage of truthiness and make this expression shorter, but it would still be longer.

@lgalfaso
Copy link

lgalfaso commented Mar 9, 2015

I have to agree with @pkozlowski-opensource, the forgiving nature of Angular 1 expressions worked well, and should be kept.

Note: Adding ?. would imply that ?[...] and ?(...) should also be added

@mhevery
Copy link
Contributor Author

mhevery commented Mar 10, 2015

We would fallow this proposal: https://github.com/gbracha/nullAwareOperators/blob/master/README.md

@ewinslow
Copy link

My team has been bitten during refactorings by the default behavior of 1.x (renamed some property but get no warning when rendering the template). Would love to see that default change and have ?. be a way to opt back in for people who really need the behavior.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 11, 2015

@ewinslow, this is a great point. The tools should be able to produce static warnings for both null-safe and strict property access when you are accessing typed objects. However, POJOs and maps would still have this issue.

@martinmcwhorter
Copy link

It seems that type-checking template linters (none exists that I know of, yet) would have an easier time with the 1.x behavior.

I think we would all like IDE or gulp/grunt linters that could plug into the TypeScript compiler service and validate user.location.address i.e. that user: User has a property location: Location which has a property address.

Not that this would be impossible with the nullAwareOperator. It would just be easier to implement such a tool without the explicit nullAwareOperator.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 23, 2015

@martinmcwhorter IDE support (or general toolability) is one of the goals of Angular 2. This is also why we are making sure that Angular 2 compiler works outside the web browser. This means that IDEs would be able to reuse the code that Angular itself uses to interpret the expressions.

@martinmcwhorter
Copy link

@yjbanov sounds good. I am a bit off topic at the moment. Right now a hole in using statically typed data/message contracts is in the templates. With TypeScript breaking changes can easily be detected in CI. Unfortunately this falls down when the types are expressed in templates. The ability to have tooling to compile these expressions and then validate them against the TS compiler service would fill a large gap.

vicb added a commit to vicb/angular that referenced this issue May 26, 2015
vicb added a commit to vicb/angular that referenced this issue May 26, 2015
vicb added a commit to vicb/angular that referenced this issue May 26, 2015
vicb added a commit to vicb/angular that referenced this issue May 26, 2015
@PatrickJS
Copy link
Member

@vicb I meant that as a response to why we should add it. One argument was that angular was extending javascript which is true since it's surprising to the developer to learn about these extensions in the binding syntax. I was only suggesting that the decision to add extensions has already been made with Pipes. Pipes is not javascript syntax and it is surprising to the developer rather than assuming it's just javascript because it's not. I was saying that argument only stands ground if you also argue to refactor pipes (and elvis) into a macros then it's simpler for the developer to reason about why we have new syntax (because they're just macros). The macro thing is whole other argument which is why we should just add elvis since it's not breaking any of the assumed angular conventions (because we already have Pipes)

vicb added a commit to vicb/angular that referenced this issue May 27, 2015
vicb added a commit to vicb/angular that referenced this issue May 28, 2015
vicb added a commit to vicb/angular that referenced this issue May 28, 2015
vicb added a commit to vicb/angular that referenced this issue May 28, 2015
vicb added a commit to vicb/angular that referenced this issue May 28, 2015
@vicb vicb removed the in progress label May 28, 2015
kegluneq pushed a commit to kegluneq/angular that referenced this issue Jun 3, 2015
Adds support for ?. to pregenerated Dart Change Detectors.

Closes angular#791
kegluneq pushed a commit to kegluneq/angular that referenced this issue Jun 3, 2015
Adds support for ?. to pregenerated Dart Change Detectors.

Closes angular#791
@leonard84
Copy link

@vicb @mhevery Sorry for resurrecting this but I want to point out that the safe-navigation-operator and the "Elvis" operator are two different things. See Wikipedia , Groovy-Elvis, Groovy-Safe-Navigation. The Elvis operator is more akin to the Null coalescing operator with the difference that it tests for "truthy" instead of null for the lhs.

displayName = user.name ? user.name : 'Anonymous'   
displayName = user.name ?: 'Anonymous'                  // <- Elvis operator

I think it is bad practice to take established terminology from other languages and change its meaning. What angular currently has is a safe-navigation-operator and not the Elvis operator. Calling it Angular “Elvis” operator ( ?. ) does not make it better ;)

@PatrickJS
Copy link
Member

also related microsoft/TypeScript#16 but it seems that it's out of scope for typescript and needs to have an ES proposal

@Elephant-Vessel
Copy link

I agree with leonard84, please rename this properly. All the different null'ish operators are confusing enough as they are.

@mhevery
Copy link
Contributor Author

mhevery commented Apr 8, 2016

@wardbell Could we rename this operator per #791 (comment) in our docs?

@christopherthielen
Copy link

See also #6387

@wardbell
Copy link
Contributor

It's on my list, thanks

@trickpattyFH20
Copy link

trickpattyFH20 commented Nov 22, 2016

Atom syntax highlighting breaks when interpolated expressions use the "safe navigation operator". Does anyone have an Atom syntax theme or package that supports the correct syntax highlighting for the "safe navigation operator" in html interpolated expressions?

@ishan123456789
Copy link

How to use it with object having special character keys viz. a?.b?['i-have-dash']

@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 Sep 23, 2019
AleksanderBodurri pushed a commit to AleksanderBodurri/angular that referenced this issue Jul 13, 2021
…r#828)

Fix angular#791

Build the render tree starting from the root node of the application.
This fix is applicable only for v12+ apps that are using the latest
debugging APIs.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.