[Bug] Object.keys() was called on the positional arguments array for a helper, which is not supported

[boris-petrov]

🐞 Describe the Bug

Calling Object.keys (or Object.values which internally uses it) on the positional arguments array of a helper does not work.

🔬 Minimal Reproduction

import Helper from '@ember/component/helper';

export default class FooHelper extends Helper {
  compute(positional) {
    Object.values(positional);
  }
}

😕 Actual Behavior

Object.keys() was called on the positional arguments array for a helper, which is not supported. This function is a low-level function that should not need to be called for positional argument arrays. You may be attempting to iterate over the array using for...in instead of for...of.

🤔 Expected Behavior

This to work.

🌍 Environment

• Ember: 3.24.1

➕ Additional Context

Found when implementing this and mentioned here.

[rwjblue]

The error does seem appropriate to me. Why would you use Object.values (which is roughly the same as a for...in loop + own property check) on an array? You already know it is an array, no need to call Object.values() on it. 🤔

[boris-petrov]

@rwjblue - well, native JS allows that. The error is an Ember one. Also, you personally requested an issue to be opened on this. 😄

[rwjblue]

Haha, indeed I did! We could possible figure out how to avoid the error, but it does seem pretty bizarre to use Object.keys specifically on an array (and Object.values() on an array also seems quite odd since you already have an array).

[boris-petrov]

@rwjblue - you're definitely right that it is a strange use. For an example as to when one might do that check this comment.

Also, note that it's just strange/unnecessary to have this assertion. Why does Ember have it? What's the problem with doing that? It's just plain JS. That's my point. Not specifically that it is particularly useful to call Object.keys on an array. :)

[pzuraq]

@boris-petrov since we are using a native Proxy, it is not just plain JS. We actually do need to write code here, one way or the other.

When I made the call on this, my thinking was: This is a tradeoff between supporting a bad pattern that is inefficient, and shipping more code to the client to support said pattern, or asserting and telling the user to do a better pattern and shipping less code to the client.

Also if we did support Object.keys, we would then have to support it indefinitely, it would be a breaking change to remove. By contrast, not supporting it always means we can add support later. So, that weighed into my decision too.

I don't have a super strongly held opinion here. I admit it's probably a little surprising the first time you run into it, but it also prevents you from iterating an array in a very slow way, compared to the alternative.

[boris-petrov]

@pzuraq - I fully agree with what you wrote. I would do this - if the added code is up to 10 lines of code, I would add it - that's not too much to support (and people won't be surprised when they try doing something similar to what I wrote above; and you would support something that is just JS - native proxies are also just JS 😄). If it's more, then leave it as is.

In any case I'll close the issue. Thanks for the discussion!