Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

Fix #89 Hooks cannot remove inherited properties #90

Merged

Conversation

supasate
Copy link
Contributor

Fix #89 issue

if (!obj || !obj.hasOwnProperty(args[i])) {
return false;
function findFieldOwner(obj, field) {
const nestedFields = field.split('.');
Copy link
Member

@daffl daffl Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just a longer way of saying !obj || typeof obj[args[i]] === 'undefined'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using typeof obj[args[i]] === undefined, we will not know whether the property is belong to the current obj or ancestor prototype (direct owner). To delete that property, we need to delete it from the direct owner. So, we need to use hasOwnProperty.

(About the const name nestedFields, I just make it more explicit but I can just use args[i] if you think it's more concise.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the code in my new commit to make it looks cleaner and more concise. Plz take a look. (I'll squash it later).

@daffl
Copy link
Member

daffl commented Aug 14, 2016

Are you sure that delete only works on the actual object? I just tried:

const o = { test: 'me' }
const oo = Object.create(o);

delete oo.test;

console.log(oo);

And I am getting an empty object logged.

@supasate
Copy link
Contributor Author

Yes. I'm sure. It's hidden in __proto__.
image

@supasate
Copy link
Contributor Author

It's the effect of prototype inheritance.

If the delete operator succeeds, it removes the property from the object entirely. However, if a property with the same name exists on the object's prototype chain, the object will inherit that property from the prototype.

Reference

@daffl daffl merged commit 48cac47 into feathersjs-ecosystem:master Aug 16, 2016
@daffl
Copy link
Member

daffl commented Aug 16, 2016

Done. Released in v1.5.6. Thank you for bearing with me 😄

@supasate
Copy link
Contributor Author

Never mind @daffl. It took me several days before figuring it out too!

@supasate supasate deleted the fix-remove-inherited-fields branch August 16, 2016 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants