Skip to content

Make the $key and $exists properties non-enumerable #535

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

Closed
wants to merge 6 commits into from
Closed

Make the $key and $exists properties non-enumerable #535

wants to merge 6 commits into from

Conversation

meDavid
Copy link

@meDavid meDavid commented Sep 20, 2016

Making these keys non enumerable makes sure that you can use Object.keys() and Object.values on the unwrapped value without having the extra properties to account for.

This would also resolve #302

Making these keys non enumerable makes sure that you can use Object.keys() and Object.values on the unwrapped value without having the extra properties to account for.
@davideast
Copy link
Collaborator

@meDavid This is awesome! Can you write a unit test to verify the behavior?

@katowulf
Copy link
Contributor

katowulf commented Sep 20, 2016

Good stuff! c[]c[] @davideast what version of IE do we support? I'm pretty sure 8 (maybe 9) needs a polyfill for this.

@davideast
Copy link
Collaborator

@katowulf Angular 2 only supports back to IE10.

@katowulf
Copy link
Contributor

Perfect. We just need test units then, @meDavid

@katowulf
Copy link
Contributor

Any chance we can get the test units in for this?

@meDavid
Copy link
Author

meDavid commented Sep 28, 2016

@katowulf Yes, I will make it happen in the next couple of days

@theunreal
Copy link

Very useful. Any updates regards this?

@meDavid
Copy link
Author

meDavid commented Nov 18, 2016

@katowulf @davideast I updated the pullrequest with an additional test to see if the behavior is correct. If you have any suggestions for improvements, let me know.

@davideast
Copy link
Collaborator

@meDavid Thank you so much! There appears to be a merge conflict, however. Can you get that sorted out and then we'll do a review to merge in?

@meDavid
Copy link
Author

meDavid commented Nov 18, 2016

@davideast I don't see any merge conflict, only a rebase conflict but I don't know how to resolve that. Any pointers?

@davideast
Copy link
Collaborator

@meDavid I'm not 100% either, but you can try to follow this guide here or simply store your changes locally, resync the fork/delete it and re-fork, and then reapply your changes.

@davideast
Copy link
Collaborator

@meDavid Any updates on this? I ran into this recently myself and I just deleted my fork and moved my changes over to the new one. You lose history, but it works.

@byrondover
Copy link
Contributor

Patched these changes in locally, works brilliantly! C'mon @meDavid, new branch and new PR to quick fix, you deserve the commit creds.

@meDavid
Copy link
Author

meDavid commented Jan 13, 2017

@byrondover I'll find some time this weekend

@meDavid
Copy link
Author

meDavid commented Jan 23, 2017

superseded by #787

@meDavid meDavid closed this Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove $key and $value on calls to set
6 participants