-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix($parse): return 'undefined' if a middle key's value is null #5480
Conversation
@@ -1036,7 +1036,7 @@ function getterFn(path, options, fullExp) { | |||
var code = 'var p;\n'; | |||
forEach(pathKeys, function(key, index) { | |||
ensureSafeMemberName(key, fullExp); | |||
code += 'if(s == null) return s;\n' + | |||
code += 'if(s == null) return undefined;\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I should check if this is the last index or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. we want to return undefined only if we are not at the last segment of the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it turned out not to be necessary, for whichever reason (istanbul would probably explain why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's perfect like that.
The return of the last property is actually outside that forEach, on it's own
So return undefined
makes perfect sense, since all these += 'ifs'
target properties that are in the middle
The only real consequence of this is that when csp is off, if the scope passed to $parse
is null
, it will return undefined
//csp off
$parse('a.b.c')(null) === undefined
//csp on (and off before this change)
$parse('a.b.c')(null) === null
So really, if we care about that, the code should be
code += 'if(s == null) return ' + (!index ? 's' : 'undefined') + ';\n' +
making us avoid breaking changes (if you're curious about the code when csp is on, there you go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the answer --- but given the unrolled test cases, I think each of these situations should be covered (well. not quite actually, but it would be an extra 6 lines to fix)
... Yeah, so even if the s == null && !index
, it returns the expected result, so I think it's probably okay without a change. not worth fixing until it's known to be broken right? edit after re-reading, I see what you're saying... hmm. I don't know if it's really worth it, but I'll wait to hear back from someone about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine
Prior to this fix, $parse/$eval would return 'null' if a middle key in an expression's value is null, when it should be expected to be undefined. This patch tries to remedy this by returning undefined for middle values in expressions, when fetching a child of that null value. For example: ```js // Given the following object: $scope.a = { b: null }; // $scope.$eval('a.b.c') returns undefined, whereas previously it would return null ```
Prior to this fix, $parse/$eval would return 'null' if a middle key in an expression's value is null, when it should be expected to be undefined. This patch tries to remedy this by returning undefined for middle values in expressions, when fetching a child of that null value. For example: ```js // Given the following object: $scope.a = { b: null }; // $scope.$eval('a.b.c') returns undefined, whereas previously it would return null ``` Closes angular#5480
Prior to this fix, $parse/$eval would return 'null' if a middle key in an expression's value is null, when it should be expected to be undefined. This patch tries to remedy this by returning undefined for middle values in expressions, when fetching a child of that null value. For example: ```js // Given the following object: $scope.a = { b: null }; // $scope.$eval('a.b.c') returns undefined, whereas previously it would return null ``` Closes angular#5480
Prior to this fix, $parse/$eval would return 'null' if a middle key in an expression's value is null, when it should be expected to be undefined.
This patch tries to remedy this by returning undefined for middle values in expressions, when fetching a child of that null value.
For example:
Fixes #2249
Fixes #5442