cleanup _getPath
#15692
cleanup _getPath
#15692
Conversation
| @@ -74,24 +68,20 @@ export function _getPath(root, path) { | |||
| let parts = path.split('.'); | |||
|
|
|||
| for (let i = 0; i < parts.length; i++) { | |||
| if (!isGettable(obj)) { | |||
| if (obj === undefined || obj === null || obj.isDestroyed) { | |||
There was a problem hiding this comment.
I don't think we want to hard code .isDestroyed check here. In theory it would be better to let m = peekMeta(obj); m.isDestroyed().
There was a problem hiding this comment.
it is how done before https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/property_get.js#L83 <- just moved this check to top
I think checking isDestroyed with meta worth another PR since hardcoded isDestroyed check is done in more places
There was a problem hiding this comment.
I will do follow up PR to use meta.isDestroyed() does it sound good ? or should I do it here
There was a problem hiding this comment.
Ya, sounds good to do separately
| @@ -5,12 +5,6 @@ | |||
| import { assert } from 'ember-debug'; | |||
| import { isPath } from './path_cache'; | |||
|
|
|||
| const ALLOWABLE_TYPES = { | |||
There was a problem hiding this comment.
why to have it :P, even if other types passed get will return undefined , get is working without ALLOWABLE_TYPES why to have it in _getPath
There was a problem hiding this comment.
Generally speaking, its best to review the git history to understand a given bit of code's purpose.
- getWithDefault broken with chained properties #13444
- [BUGFIX beta]
Ember.getwith first part of path chain resolving tonullreturnsnullrather thanundefined#13449 - Prevent ungettable objects from getting gotten during path lookup #13461
Specifically answering your question though is #13449 (comment).
There was a problem hiding this comment.
added test for those cases if something missing will add it, I think all cases handled correctly
21f4054 to
28c902d
Compare
28c902d to
3096fec
Compare
3096fec to
595f5ad
Compare
595f5ad to
1b6bc3c
Compare
1b6bc3c to
860561c
Compare
|
ping, any objections to this ? |
| @@ -178,24 +172,16 @@ export function _getPath<T extends object>(root: T, path: string): any { | |||
| let parts = path.split('.'); | |||
|
|
|||
| for (let i = 0; i < parts.length; i++) { | |||
| if (!isGettable(obj)) { | |||
| if (obj === undefined || obj === null || (obj as MaybeHasIsDestroyed).isDestroyed) { | |||
There was a problem hiding this comment.
maybe move (obj as MaybeHasIsDestroyed).isDestroyed to get itself ?
No description provided.