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

prop.get(user, 'name.first') will throw if name is null #11

Closed
matthewmueller opened this issue Nov 14, 2015 · 9 comments
Closed

prop.get(user, 'name.first') will throw if name is null #11

matthewmueller opened this issue Nov 14, 2015 · 9 comments

Comments

@matthewmueller
Copy link

Seems weird to me that this library throws on things like: null, false, 3, "hi".

I feel like the obj === undefined check should be isObj(obj) or something.

@stevemao
Copy link
Collaborator

Maybe we should check the accessibility of the property?

@sindresorhus
Copy link
Owner

Yeah, that's a bug. It should never throw no matter what you feed it. That's the whole point of this module.

@stevemao
Copy link
Collaborator

@sindresorhus do you want to support situation that a getter might throw? the only way is to try... catch it?

@sindresorhus
Copy link
Owner

Yes

@stevemao
Copy link
Collaborator

I think I know what you are going to say @sindresorhus , you probably just don't want to 'throw' from this module (and don't care too much about the performance) but the reason I try to avoid 'try...catch' and this module to 'throw' was to avoid regression.

Sent from my iPhone

On 16 Nov 2015, at 8:35 PM, Sindre Sorhus notifications@github.com wrote:

Yes


Reply to this email directly or view it on GitHub.

@sindresorhus
Copy link
Owner

and don't care too much about the performance

try/catch doesn't have to mean worse performance. try/catch only deoptimizes the current function. If you put it in a helper function you're good.

@stevemao
Copy link
Collaborator

Ok, I'll get my Mac next Monday so seven more days if you guys don't mind waiting :)

@sindresorhus
Copy link
Owner

I can wait. I don't have time to look into this right now anyways.

@stevemao
Copy link
Collaborator

@matthewmueller @sindresorhus please review #12 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants