-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX lts] Ensure {{each-in}} can iterate over keys with periods #18296
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,14 @@ | ||
import { get, objectAt, tagFor, tagForProperty } from '@ember/-internals/metal'; | ||
import { | ||
consume, | ||
get, | ||
isTracking, | ||
objectAt, | ||
tagFor, | ||
tagForProperty, | ||
} from '@ember/-internals/metal'; | ||
import { _contentFor } from '@ember/-internals/runtime'; | ||
import { guidFor, HAS_NATIVE_SYMBOL, isEmberArray, isProxy } from '@ember/-internals/utils'; | ||
import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features'; | ||
import { assert } from '@ember/debug'; | ||
import { | ||
AbstractIterable, | ||
|
@@ -127,7 +135,22 @@ class ObjectIterator extends BoundedIterator { | |
} else { | ||
let values: Opaque[] = []; | ||
for (let i = 0; i < length; i++) { | ||
values.push(get(obj, keys[i])); | ||
let value: any; | ||
let key = keys[i]; | ||
|
||
value = obj[key]; | ||
|
||
// Add the tag of the returned value if it is an array, since arrays | ||
// should always cause updates if they are consumed and then changed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that right? If you pass this to, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test you added should pass without consuming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of autotracking as a whole, it allows consumers to use arrays (both native and non-native) anywhere they consume a tracked value, and have mutations to that array be autotracked. In practice, this is like the native getters migration, it’ll mean users can access any array as if it were a normal, native array. They only need to use methods for updating the state of the array (we need native Proxy to allow them to update with native syntax). Even when we do get to native Proxy though, this will allow us to omit a |
||
if (EMBER_METAL_TRACKED_PROPERTIES && isTracking()) { | ||
consume(tagForProperty(obj, key)); | ||
|
||
if (Array.isArray(value) || isEmberArray(value)) { | ||
consume(tagForProperty(value, '[]')); | ||
} | ||
} | ||
|
||
values.push(value); | ||
} | ||
return new this(keys, values, length, keyFor); | ||
} | ||
|
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.
This breaks when
obj
is a proxy, is that okay?In general, if the solution requires avoiding
get
semantics, I think that is not okay. However, maybe in this case it is fine since we already didObject.keys
above so it won't triggerunknownProperty
anyway.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.
Correct, I started writing the code to do
unknownProperty
and realized that by definition, it could never run, which is why I omitted it.