Skip to content

Correct bug in unique #108

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

Closed
wants to merge 1 commit into from
Closed

Correct bug in unique #108

wants to merge 1 commit into from

Conversation

inanna-malick
Copy link

This fixes an issue where [].unique() evaluated to [null].

For some godawful reason

console.log(JSON.stringify(value) + " => " + JSON.stringify(value)) for key, value of {}

logs

undefined => undefined 

So we must ensure we only return properties owned by the output object, so that null doesn't get added to our array of unique elements. This issue may be specific to my setup (Latest version of Chrome, running on a Windows VM in close proximity to an Indian burial ground), but I believe this change still improves the clarity and correctness of the example.

For some godawful reason 
```
console.log(JSON.stringify(value) + " => " + JSON.stringify(value)) for key, value of {}
```
logs
```
undefined => undefined 
```

So we must ensure we only return properties owned by the output object, so that null doesn't get added to our array of unique elements. This issue may be specific to my setup (Latest version of Chrome, running on a Windows VM in close proximity to an Indian burial ground), but I believe this change still improves the clarity of the example.
@michaelglass
Copy link
Member

I can't repro on 36.0.1952.0 (Official Build 265233) canary 34.0.1847.116 (Official Build 260972) (for key, value of {} never enters the loop). Can you link a jsfiddle with your compiled coffeescript in case there are differences?

@sukima
Copy link
Contributor

sukima commented Apr 23, 2014

Which Pull Request is the one your working on? Seems like there are three (Issue #106, Pull Requests #107 and #108) And in #107 you refered to this being hacky and will make a fourth soon.

@inanna-malick
Copy link
Author

I'm away from my machine right now, I'll put a jsfiddle together tomorrow to demonstrate this. (Or update you on how exactly I shot myself in the foot & caused this to happen. Either way.)

@lolmaus lolmaus closed this Jan 9, 2015
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.

4 participants