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

Hoek.reach and non standard JS objects #152

Closed
MathieuLoutre opened this issue Aug 13, 2015 · 11 comments
Closed

Hoek.reach and non standard JS objects #152

MathieuLoutre opened this issue Aug 13, 2015 · 11 comments
Assignees
Labels
Milestone

Comments

@MathieuLoutre
Copy link

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

I've posted on the hapijs repo before and it explains the situation in detail: hapijs/hapi#2644

Mongoose objects likely override . and [] notations for access because Object.getOwnPropertyNames(ref) does not reveal any of the object attributes. Because of that, Hoek.reach struggles to get to the particular attribute when it's something like mongooseObject.attribute.

I realise it's a pretty specific use case, but if there were a way to adapt the code so it still works, it would be brilliant.

From the hapijs issue:

I found that the Mongoose object doesn't seem to return anything on ref.hasOwnProperty(path[i]) and therefore fails. However, ref[path[i]] does exist and returns something. Checking for ref[path[i]] instead of ref.hasOwnProperty(path[i]) does not fix the problem, but maybe there's a solution.

@Marsup Marsup added the request label Aug 13, 2015
@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Aug 13, 2015

https://github.com/hapijs/hoek/blob/master/lib/index.js#L599

ref.hasOwnProperty(key) => Object.getOwnPropertyDescriptor(ref, key)

@MathieuLoutre

This comment has been minimized.

Copy link
Author

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

Interesting idea but I just tried it locally and it still returns undefined.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Aug 13, 2015

Are you sure you didn't forget the bang ?

@MathieuLoutre

This comment has been minimized.

Copy link
Author

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

Yes, pretty sure. However, just realised that I was working on an older version of Hoek. Will try on the latest and see what happens.

@MathieuLoutre

This comment has been minimized.

Copy link
Author

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

Upgraded to latest with L599 looking like this: !Object.getOwnPropertyDescriptor(ref, key) || and still the same issue (returns undefined).

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Aug 13, 2015

You'll have to dig by yourself, getOwnPropertyDescriptor should get anything if it exists, I don't see how it could not work.

@MathieuLoutre

This comment has been minimized.

Copy link
Author

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

ref[key] isn't undefined. I suspect the properties are stored in a sub-object (like .attributes) and accessed by overriding [] and .. Checking for ref[key] isn't a direct replacement for ref.hasOwnProperty(key) so it's not backwards compatible.

How about checking both?

L599 as !(ref.hasOwnProperty(key) || ref[key]) || solves the problem on my side and the tests are still passing.

I can write the PR if that sounds good.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Aug 13, 2015

Maybe asking the mongoose author would be the best way forward.

@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Aug 13, 2015

By design, the hasOwnProperty lookup won't find all properties of an object. Specifically, it omits properties in the prototype chain, like toString.

I suspect changing it to !(key in ref) will work.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Aug 13, 2015

getOwnPropertyDescriptor doesn't have that issue and it still won't find anything. I don't know what trick they're using.

EDIT: nvm, misread the docs, it won't go through the proto chain.

@MathieuLoutre

This comment has been minimized.

Copy link
Author

@MathieuLoutre MathieuLoutre commented Aug 13, 2015

@kanongil Interestingly, L599 as !(path[i] in ref) || seems to work.

@nlf nlf added this to the 3.0.4 milestone Nov 14, 2015
@nlf nlf self-assigned this Nov 14, 2015
@nlf nlf closed this in 88a6731 Nov 14, 2015
@Marsup Marsup added feature and removed request labels Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.