Support Symbol named observable properties #1944
Conversation
| export function getPlainObjectKeys(object) { | ||
| return unique( | ||
| ([] as PropertyKey[]).concat.apply( | ||
| [], |
There was a problem hiding this comment.
I think this implementation is incorrect, the for ... in loop returns properties from the entire prototype chain (not just the direct prototype, as in the current one), but only the ones that are enumerable (unlike getOwnPropertySymbols), so this implementation needs to be refined a little further.
There was a problem hiding this comment.
I think however that in none of the cases it is needed to grab keys from the prototype, so Reflect.keys, and filtering out the enumerable ones, should do the trick (with e.g. propertyIsEnumerable).
A more accurate name would then be getOwnEnumerablePropertyNames.
After a quick inspection of the usage sides, the only function that might run into trouble by not inspecting prototype chain might be toJS, it would be good to test that.
There was a problem hiding this comment.
Cool I'll run with these suggestions, thanks mate! I'll get back with changes as soon as I can.
I did observe in toJS tests that we are expecting enumerable properties up the prototype to be output, this initial implementation was to address those failing tests - I can split a test or two out to make this intention a little bit more obvious for us.
There was a problem hiding this comment.
Pushed up what I think is now the correct implementation, following your advice! Ty! 😄
| const objKeys = getPlainObjectKeys(source) | ||
| for (let i in objKeys) { | ||
| const key = objKeys[i] | ||
| if ( |
There was a problem hiding this comment.
I think these exception are no longer needed once non-enumerable symbol properties are filtered out (see below)
|
That would be great!
…On Thu, Apr 18, 2019 at 2:54 AM Lochlan Bunn ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/utils/utils.ts
<#1944 (comment)>:
> @@ -150,6 +150,27 @@ export function isES6Set(thing): thing is Set<any> {
return thing instanceof Set
}
+/**
+ * Returns the following: own keys, prototype keys & own symbol keys.
+ */
+export function getPlainObjectKeys(object) {
+ return unique(
+ ([] as PropertyKey[]).concat.apply(
+ [],
Cool I'll run with these suggestions, thanks mate! I'll get back with
changes as soon as I can.
I did observe in toJS tests that we are expecting enumerable properties
up the prototype to be output, this initial implementation was to address
those failing tests - I can split a test or two out to make this intention
a little bit more obvious for us.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1944 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAN4NBDLT552VE5XVS6L2C3PQ7BFNANCNFSM4HDMCN5A>
.
|
|
Hi friends. What's the status of this patch? @loklaan @mweststrate |
|
I think @loklaan didn't push any new changes after the review? |
* Add tests for using symbol keys and getting them from toJS
loklaan
force-pushed
the
feature/symbol-support
branch
from
94d812d to
421e793
Compare
|
@xuchaobei Slow turn around on my end 😅 I hope the changes I just pushed up (rebased on latest |
Hey folks,
I was recently stuck trying to figure out why my observable properties weren't working with Symbols as keys. Then I found #1809 & #1925.
In this PR I hope to get things kicked off. Being new to the codebase there are probably thing's I've missed.
I've also tested this is a mid size production app.
PR checklist:
gh-pagesbranch. Please refer to this PR). For new functionality, at least API.md should be updatednpm run perf)