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

Support object keys in addition to arrays #30

Closed
briancavalier opened this issue Apr 25, 2012 · 24 comments
Closed

Support object keys in addition to arrays #30

briancavalier opened this issue Apr 25, 2012 · 24 comments
Milestone

Comments

@briancavalier
Copy link
Member

On several occasions, I've wanted to use when.reduce() on an object's keys rather than on an array, and @sifu recently tweeted something similar about when.all(). Also, promised-io provides allKeys(), in parallel to all().

It'd be great to find an elegant way to support object keys across all the when.js "array" methods. One approach might be to create equivalent "keys" methods: allKeys(), reduceKeys(), etc., or to provide an additional module that houses the object key versions of those methods.

Not sure what is best here, so looking for input, keeping in mind that staying as compact as possible is an important goal :)

@sifu
Copy link

sifu commented Apr 25, 2012

i would be in favor of having them in the same module. when i use promised-io i use allKeys( ) more often than the array methods.

but no matter where they end up, i will be happy :)

@briancavalier
Copy link
Member Author

Another namespacing option: when.keys.all(), when.keys.reduce(), etc. Not sure if "keys" is the right word there, but if we decide to put the key-based methods into the same module, I kind of like the idea of having a sub-namespace.

This also makes me wonder if it's possible to generate the keys methods from their corresponding array methods. I.e. is it possible to generate when.keys.all() from when.all() ...

@unscriptable
Copy link
Member

I like the separate module idea. If the key-ified module were to load when as a dependency, you could use AMD path trickery:

curl({ packages: [ { name: 'when', location: 'path/to/when', main: 'when/keys' }); // or whatever method your AMD loader uses
define(['when'], function (when) {
    when.keys.all(objectWithPromisesAsValues, doSomething);
});

@briancavalier
Copy link
Member Author

Interesting idea. Are you thinking that when/keys would load when as a dependency, and then decorate it with a keys namespace and methods?

@sifu
Copy link

sifu commented Apr 26, 2012

i like the namespace idea! but i'm also not sure if "keys" is the right word. when i discovered the allKeys( ) function in promised-io i had to look up the documentation because i had no clue about what that could be.

@jonnyreeves
Copy link

+1 for the namespace idea, it avoids cluttering the core when API and would also all for orthogonality within the new module. I think keys is a pretty good name; but maybe something like when.hashKeys.reduce() is more descriptive - bit of a tricky one :)

@briancavalier
Copy link
Member Author

Cool, thanks for the feedback on the namespace approach. It's probably the direction we'll ... just need to decide on a name :) Some other options:

  1. when.keys
  2. when.props or when.properties
  3. when.object

Other ideas?

@briancavalier
Copy link
Member Author

Here's another option. What if when.all/any/some/map/reduce included a test to see if they are dealing with an array or an object, and just "did the right thing"? What are the pros and cons? Some initial thoughts:

Pros:

  1. Very easy to use: no need to require() another module, the functionality is always available in the core when module
  2. No need to think about which version of a method to call, e.g. when.all() vs. when.props.all(), just use when.all()

Cons:

  1. There's a potentially non-trivial code size hit (hundreds of bytes, most likely) if you don't need the object functionality. It will always be loaded.
  2. There's a very minor performance hit in each call to when.all/any/some/map/reduce to check for array vs. object. Honestly, this seems like a non-issue. Any async operation, like XHR, will dominate performance by many orders of magnitude.
  3. Potential risk of "oops": thinking you have an array when you have an object, and passing it to when.all() and it does something you didn't expect. Right now, this seems like a non-issue to me, too. If you think you have an array, and you really have an object, your code is probably broken anyway ;)

Thoughts?

@briancavalier
Copy link
Member Author

I just pushed a branch that integrates object key support directly into when.all/any/some/map/reduce. So, you can pass an object or a promise for an object as the first param to any of the above methods, and it will operate on the object's keys/props. Arrays (and promises for arrays) should still all work as before (the unit tests say they do, anyway!)

I'm sure there are plenty of botches and edge cases I haven't covered, so the usual caveats apply :) Please try it out and let me know what you think of this versus potentially providing a separate module for object prop support.

@KidkArolis
Copy link
Contributor

Any reason this hasn't made it to the master?

@briancavalier
Copy link
Member Author

Mostly because we've been focused on getting version 2.0 ready, and on other parts of cujo. I think we could probably get this into a 2.1 release. Do you have an immediate need for it?

@KidkArolis
Copy link
Contributor

We found it to be useful sometimes. We've implemented our own version, but
would be nice to have it available out of the box so to say. 2.1 sounds
good.

I wonder if this feature would overcomplicate things. I guess not, if you
think about it's quite intuitive in underscore.js to know when a certain
function will accept both an array and an object. It should be similar here.
On Mar 8, 2013 12:32 AM, "Brian Cavalier" notifications@github.com wrote:

Mostly because we've been focused on getting version 2.0 ready, and on
other parts of cujo. I think we could probably get this into a 2.1 release.
Do you have an immediate need for it?


Reply to this email directly or view it on GitHubhttps://github.com//issues/30#issuecomment-14595848
.

@briancavalier
Copy link
Member Author

Ok, cool. Yeah, I go back and forth on whether we should integrate it directly into when.all/etc. Lately, I've been feeling like single-purpose methods & functions are the way to go, so, at least right now, I feel like a separate when/keys module with all, map, and reduce methods would be a good path.

@briancavalier
Copy link
Member Author

@KidkArolis You inspired me :) How does this look? ef1eccb

@KidkArolis
Copy link
Contributor

Nice! I'm not exactly sure why it's called when/keys. Although, the only alternative I can think of is when/object.

Also, how would you name this module in your code? I suppose:

var whenKeys = require("when/keys");
whenKeys.all({...});

@briancavalier
Copy link
Member Author

Here's my thinking on the name when/keys. I considered a few names: when/keys, when/object, and when/props. I decided against when/props because I felt it should behave like Object.keys. Encouraging people to think of the object they pass to these new methods as a simple map or hash seems safer than allowing them to pass complex objects with deep prototype chains that may contain promises--that seems fraught with pitfalls to me.

I do like when/object, and it'd make a nice parallel if we ever extract the current array methods to a when/array module. There are 2 reasons I decided to go with when/keys over when/object. First, I think people will tend to name vars like the module name, i.e. var keys vs. var object, or var whenKeys (like you said) vs. var whenObject. Given that, to me, keys.all/map/reduce reads a bit more intuitively than object.all/map/reduce. I feel it communicates specifically that, for example, keys.map will map the keys of whatever you pass to it. Second, I feel it's important to imply that it will only map keys (i.e. own properties), and won't dig into the prototype chain. I think the name when/keys implies that more strongly than when/object.

All of that said, I'm def open to other names. I'm even open to rethinking the own props vs. prototype chain decision, if there are (very) compelling reasons.

@briancavalier
Copy link
Member Author

Hmmm, does keys.reduce actually make sense, given that a typical reduce/fold is ordered, and there is no officially required key enumeration order in EMCAScript? In practice, most engines use key insertion order, but there's no real reason it would be consistent.

I can still see it being useful, so maybe the right thing is just a BIG DISCLAIMER in the docs?

@KidkArolis
Copy link
Contributor

Thanks for sharing your thoughts! I like the naming of when/keys now :)

I would consider removing keys.reduce though? I try to avoid relying on the order in such cases, why allow (encourage?) others to do it by providing this function. I don't think the behaviour is as consistent as you think in most engines, I'm sure I've seen some funky differences when you're adding/removing keys or using numeric keys or something, might be worth investigating more if it can lead to subtle bugs.

The prototype chain problem could be handled by a separate function when/keys/deepMap? But I would try to avoid that too until someone asks for one.. :)

@briancavalier
Copy link
Member Author

Yeah, the key ordering problem seems scary, and could cause some subtle and frustrating bugs. I know at least in Chrome there have been changes to the ordering, some of which involve differences around numeric keys, like you pointed out.

You've very nearly convinced me that forcing people to do something like keys.all(obj).then(function(resolvedMap) { Object.keys(resolvedMap).reduce(...); }); is the way to go.

I'll let it stew a bit, and see how I feel.

I agree about deepMap. If someone presents a strong case for it, we can consider it in a separate issue.

@briancavalier
Copy link
Member Author

Ugh, underscore and lodash both support reducing object keys :/

@KidkArolis
Copy link
Contributor

We could ask them why they think that's ok.

@briancavalier
Copy link
Member Author

Ok, after a quick discussion with @unscriptable, and after reading this v8 issue, I'm convinced that, at least for now, keys.reduce has too much potential for confusion. Reduce/fold is, by it's very nature an ordered operation: http://en.wikipedia.org/wiki/Fold_(higher-order_function). While something commutative, like addition or boolean and/or, will work fine with arbitrary order, something as simple as string concatenation is dangerous with non-deterministic ordering!

briancavalier added a commit that referenced this issue Mar 8, 2013
@briancavalier
Copy link
Member Author

@unscriptable did end up asking @jdalton briefly about reduce(obj) in lodash, and the general sense was that people weren't confused by it and tended not to use it for operations where non-deterministic key ordering was a problem. So maybe it's not a problem in practice.

Still, we'll probably launch 2.0 without it and happily add it later if people want (and understand the tradeoffs!)

@briancavalier
Copy link
Member Author

The when/keys module is released in when 2.0.0 with all and map. If folks have strong use cases for reduce, we'll consider adding it (with the caveat listed above about key ordering)

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

No branches or pull requests

5 participants