Skip to content
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

Improve Object Proxy #112

Closed
wants to merge 2 commits into from
Closed

Conversation

Exelord
Copy link

@Exelord Exelord commented Jun 7, 2021

Hi :)

while I was working on VZN | Reactivity I noticed several bugs related to keys tracking, so Im sharing the outcome of long investigations.

Once this will land in VZN I will also add additional tests (unless someone wants to help with that)

What has been fixed:

  • deleteProperty and defineProperty is notifying ownKeys change, individual desc's key accessed by has and the property if tracked
  • has is now tracking a single object desc's key instead the whole collection
  • set is notifying the property only if really changed, notifying the desc's key as well the collection if new desc's key has been set

@chriskrycho chriskrycho added the bug Something isn't working label Oct 26, 2021
@chriskrycho
Copy link
Collaborator

@Exelord sorry this languished for so long. Do you mind re-evaluating now that #161 has landed? It’s not totally clear to me whether the changes here are still necessary, but if so they'll need to be reimplemented using those primitives.

Additionally, if you do end up updating this, can you elaborate on why the cache is valuable here? Thanks!

@chriskrycho
Copy link
Collaborator

Going to go ahead and close this; feel free to open an issue explaining what it is you want to accomplish if you still want to tackle this and we can discuss there before actually starting more work. It'll need to be particularly well-described and well-motivated, as we're currently preparing this to be part of Glimmer and Ember proper (see #316 and emberjs/rfcs#812).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants