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

Allow getting non-enumerable properties #54

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Apr 20, 2019

Changes the way to check property for enumerability, fixes #46

@stroncium
Copy link
Contributor Author

stroncium commented Apr 20, 2019

Side note: no code changes around enumerability were ever applied to anything but get.

@sindresorhus
Copy link
Owner

I need your opinion; does it really make sense to check enumerability at all? I just read through the original issue about it (#23) and I'm not sure I buy the argument anymore. Also, it's weird that the check is only applied to get and not the other methods.

@stroncium
Copy link
Contributor Author

Con: it really does work different from normal object access(obviously ignoring whole exception vs undefined part there). But I can hardly imagine real world situations when people want to read properties flagged non-enumerable using this lib, and if they do their design will clearly considered by most to be really bad.

Pro: it actually seems to better match expectations and frequent use-cases. This bug is about inherited properties and it makes sense it should read inherited properties. For the other cases, it silently protects from sometimes hard to trace bugs.

All in all, I'd say I'm for doing enumerability checks.

Logic should probably be applied to all methods, yeah. (Though logic will always be weirdly different between reading and modifying methods for prototypes.)

@stevemao
Copy link
Collaborator

I vote for removing related code of enumerability.

@stroncium
Copy link
Contributor Author

@sindresorhus So, what are we going for there? Removal of enumerability checks or unifying method behavior?

@lselden
Copy link

lselden commented Jan 27, 2020

@sindresorhus Request to get this merged, though I understand it might qualify as a major release bump. I don't see any reason that a value's enumerable value should matter if someone explicitly tries to access it using .get / .has

@sindresorhus
Copy link
Owner

Remove the enumerability checks.

@sindresorhus sindresorhus changed the title Changes the way to check property for enumerability, fixes #46 Allow getting non-enumerable properties Oct 7, 2020
@sindresorhus sindresorhus merged commit cbd7074 into sindresorhus:master Oct 7, 2020
@stroncium
Copy link
Contributor Author

Side note, I feel like this package could use a bit more tests. Nothing exact of the top of my head, but I can't believe existing ones cover all the tricky stuff.

@sindresorhus
Copy link
Owner

@stroncium Comment in #70 if you have any ideas of what the tests might be.

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

Successfully merging this pull request may close these issues.

get() method's switch to object.prototype.propertyIsEnumerable broke my code
5 participants