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

Object.keys doesn't work correctly with string objects. #209

Closed
jdalton opened this issue Mar 25, 2014 · 10 comments
Closed

Object.keys doesn't work correctly with string objects. #209

jdalton opened this issue Mar 25, 2014 · 10 comments

Comments

@jdalton
Copy link
Contributor

jdalton commented Mar 25, 2014

See http://jsbin.com/xijifore/1.
In IE8 it will alert 0 instead of 5.

alert(Object.keys(Object("hello")).length);
@ljharb ljharb self-assigned this Mar 25, 2014
ljharb added a commit to ljharb/es5-shim that referenced this issue Mar 26, 2014
@ljharb ljharb closed this as completed in 2dd3abf Mar 26, 2014
@jdalton
Copy link
Contributor Author

jdalton commented Mar 31, 2014

I don't believe this issue was addressed.

@ljharb
Copy link
Member

ljharb commented Mar 31, 2014

I wasn't able to reproduce your issue in object-keys, and adding arguments detection makes the implementations identical. Can you still reproduce the String issue in es5-shim, and in object-keys? I'll reopen just in case.

@ljharb ljharb reopened this Mar 31, 2014
@jdalton
Copy link
Contributor Author

jdalton commented Mar 31, 2014

Before your patch es5-shim:

Can you still reproduce the String issue in es5-shim, and in object-keys?

After your patch es5-shim it throws an error in IE8 (mentioned in the commit comments).

@ljharb
Copy link
Member

ljharb commented Mar 31, 2014

Cool, thanks for reporting, I'll take a look.

@ljharb ljharb closed this as completed in 94e0add Mar 31, 2014
@jdalton
Copy link
Contributor Author

jdalton commented Mar 31, 2014

On 94e0add

var a = Object('x');
a.y = 1;
Object.keys(a);
// should be ["0", "y"]

currently it will only be ["0"]

@ljharb ljharb reopened this Apr 1, 2014
@ljharb
Copy link
Member

ljharb commented Apr 1, 2014

This is a tricky one :-) thanks, will keep looking.

@hazzik
Copy link
Contributor

hazzik commented Apr 1, 2014

I think you just need to remove else keyword?

@jdalton
Copy link
Contributor Author

jdalton commented Apr 1, 2014

@ljharb
I fixed this in Lo-Dash before reporting it here.
For reference see the compat build (you can ignore the array checks as we treat all arrays as dense).

@ljharb
Copy link
Member

ljharb commented Apr 17, 2014

This final edge case still fails in Firefox, but should pass everywhere else. I'm leaning towards this holding up a v3.0 build - since we're now overriding more native methods, it's worth a major version bump imo. Hopefully I'll have time to finish it up this weekend.

@ljharb
Copy link
Member

ljharb commented Apr 17, 2014

Actually this is now fixed. Please open a new issue if you find more issues with Object.keys :-) thanks!

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