Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($parse): stateful interceptors override an undefined expression #9825

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

When using $parse with a stateful interceptor and the expression is undefined, then return the result from the interceptor

Closes #9821

@lgalfaso
Copy link
Contributor Author

@rodyhaddad WDYT?

@lgalfaso lgalfaso force-pushed the issue-9821 branch 2 times, most recently from 12b1c69 to 3a3bc81 Compare October 29, 2014 15:24

inject(function($rootScope) {
compile('<div ng-controller="controllerWithWatch">' +
'<p>{{text}}</p>' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong, should be {{greeting}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a watcher on the parent scope that will copy greeting to text that is not triggered

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of the watcher in the parent scope, that is bogus and unrelated to the issue, honestly. The problem still happens when the 2-way binding name is used. That's bad.

When using `$parse` with a stateful interceptor and the expression
is `undefined`, then return the result from the interceptor

Closes angular#9821
@lgalfaso
Copy link
Contributor Author

updated the patch with the comments

@caitp
Copy link
Contributor

caitp commented Oct 29, 2014

yeah --- so I mean this is okay, but it's weird that we're special-casing $stateful for this. It seems like this should be the default behaviour for anything that isn't worried about bind-once.

@caitp
Copy link
Contributor

caitp commented Oct 29, 2014

and $stateful's meaning makes a lot more sense when it comes to figuring out whether to invoke filters or not. It doesn't make a lot of sense when we're talking about overriding a return value.

@caitp
Copy link
Contributor

caitp commented Oct 29, 2014

@tbosch / @jeffbcross what do you think --- leave it as is, or find a way to isolate the bogus behaviour to just bind-once delegates?

@lgalfaso
Copy link
Contributor Author

the other option would be to add another property to the interceptor to state this interceptor overrides an undefined. We cannot just remove the logic as there are other interceptors (e.g. interpolate that will transform undefined into the empty string) that would be better if left alone

@caitp
Copy link
Contributor

caitp commented Oct 29, 2014

I don't see why we'd want to do that --- the behaviour was added for bind-once, that's where it makes sense. Why add another footgun for people to use?

@petebacondarwin petebacondarwin modified the milestones: 1.3.2, 1.3.x Nov 3, 2014
@caitp
Copy link
Contributor

caitp commented Nov 7, 2014

@IgorMinar did you ever take a look at this?

As I said, I think we could land it and fix it up later, but I would prefer we didn't do it in this fashion

@IgorMinar
Copy link
Contributor

I'm not keen on having another flag.

I'm with @caitp that something smells here.

As @caitp suggested the change above should be the default behavior, but that requires some changes to one-time binding and $interpolate. I think it's doable.

The fix above shouldn't cause other issues AFAICT, so let's land it now and improve next week.

@caitp
Copy link
Contributor

caitp commented Nov 7, 2014

sounds good to me

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.