fix(utils): Make object $key and $exists properties non-enumerable #787
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it!
…On Sun, Jan 22, 2017 at 5:10 PM, googlebot ***@***.***> wrote:
Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project. Before we can look at your
pull request, you'll need to sign a Contributor License Agreement (CLA).
📝 *Please visit https://cla.developers.google.com/
<https://cla.developers.google.com/> to sign.*
Once you've signed, please reply here (e.g. I signed it!) and we'll
verify. Thanks.
------------------------------
- If you've already signed a CLA, it's possible we don't have your
GitHub username or you're using a different email address. Check your
existing CLA data <https://cla.developers.google.com/clas> and verify
that your email is set on your git commits
<https://help.github.com/articles/setting-your-email-in-git/>.
- If you signed the CLA as a corporation, please let us know the
company's name.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#787 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA1XeUw777I51Sj9ApyMIXnu9zQCE4qEks5rU_4AgaJpZM4Lqk7Q>
.
|
|
CLAs look good, thanks! |
1 similar comment
|
CLAs look good, thanks! |
|
@byrondover Thanks you for finishing this |
There was a problem hiding this comment.
@byrondover Thank you so much for doing this! Just two minor comments and then I think we can get it in. 🔥
| unwrapped.$exists = () => { | ||
| return snapshot.exists(); | ||
| }; | ||
| Object.defineProperty(unwrapped, '$key', {value: snapshot.ref.key, enumerable: false}); |
There was a problem hiding this comment.
Format this line to the following:
Object.defineProperty(unwrapped, '$key', {
value: snapshot.ref.key,
enumerable: false
});There was a problem hiding this comment.
Done! => ef09ba1
| @@ -81,12 +81,12 @@ describe('FirebaseObjectFactory', () => { | |||
|
|
|||
|
|
|||
| it('should emit unwrapped data by default', (done: any) => { | |||
| ref.set({ data: 'bar' }, () => { | |||
| const fixtureData = { data: 'bar' }; | |||
| ref.set(fixtureData, () => { | |||
| subscription = observable.subscribe(unwrapped => { | |||
There was a problem hiding this comment.
The observable subscription shouldn't be needed here. Can you remove it and just use the inner code?
There was a problem hiding this comment.
@davideast I'm not sure I follow. How can we assert the object properties after ref.set(), other than by subscribing to the object observable?
If it'll take too long to explain, please feel free to just tell me what to do here specifically, and I'm sure I'll figure it out as I implement the changes. 😎
There was a problem hiding this comment.
Sorry, my brain was malfunctioning. I looked at this too early in the morning. I forgot that there's a FirebaseObjectObservable variable in the outer scope. All is good!
Checklist
npm install,npm run build, andnpm testrun successfully? yesDescription
Exact same pull request as #535, addresses rebase merge conflicts. All credit for original fix goes to @meDavid.