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

Ember.A(null) now returns null instead of [] #13284

Closed
workmanw opened this issue Apr 9, 2016 · 5 comments
Closed

Ember.A(null) now returns null instead of [] #13284

workmanw opened this issue Apr 9, 2016 · 5 comments

Comments

@workmanw
Copy link

workmanw commented Apr 9, 2016

Here is a pretty clear snippet that explains the issue:

// Ember 2.4.x
Ember.A(null);   // returns []

// Ember Beta
Ember.A(null);   // returns null

So what happened? It looks like this commit broke it: 586d4c6#diff-7 :(. It seems very harmless, but that code is transpiled as:

A = A = function () {
  var arr = arguments.length <= 0 || arguments[0] === undefined ? [] : arguments[0];
  return arr;
};

The problem is that because Ember.A(null) has an argument length greater than 1, it's value [null] is ultimately returned.

Unfortunately this did break a few tests in our application. Specifically with the fixture adapter, see here: https://github.com/emberjs/ember-data-fixture-adapter/blob/master/addon/index.js#L298-L300 . I believe this to be part of the public API and should be fixed. If so, I'd be happy to add a test and take care of it.


EDIT: As a side note, I feel like I should voice some my concern about the aforementioned commit ( 586d4c6 ). There are several other places where argValue || [] was replaced with default arguments, potentially leading to the same issue.

@mmun
Copy link
Member

mmun commented Apr 9, 2016

Yep. This should totally be fixed and a tested added.

Out of curiosity, which version of Ember.A(null) do you think makes more sense?

@thoov can you do a quick audit of what else might have broken?

@workmanw
Copy link
Author

workmanw commented Apr 9, 2016

Out of curiosity, which version of Ember.A(null) do you think makes more sense?

@mmun I was just asking myself that. I read the API docs to see if they indicated one way or another, but it was still kind of ambiguous. I could probably make an argument either way.

I sometimes use Ember.A as a safety guard for functions expecting a list argument. Eg:

mapListValues(list) {
  return Ember.A(list).map(item => { /* ... */ });
})

But I could be convinced that this isn't the purpose of Ember.A.

workmanw pushed a commit to workmanw/ember.js that referenced this issue Apr 9, 2016
…)` returning `null` instead of an array. See issue emberjs#13284.
@workmanw workmanw closed this as completed Apr 9, 2016
@workmanw
Copy link
Author

workmanw commented Apr 9, 2016

This issue was fixed with #13287 being merged in.

@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2016

Thanks for catching this @workmanw!

@workmanw
Copy link
Author

@rwjblue My pleasure!

Random Fact: I just started using ember-try to test our app against "default", "stable", "beta" and "canary". Setting up a nightly cron to run the tests against all 4 scenarios and email me of any failures. On the first night, it caught this. If I can ever muster the cycles to write a blog post, it'll be about how to do this.

rwjblue pushed a commit that referenced this issue Apr 11, 2016
…)` returning `null` instead of an array. See issue #13284.

(cherry picked from commit 4dea78c)
toddjordan pushed a commit to toddjordan/ember.js that referenced this issue Sep 9, 2016
…)` returning `null` instead of an array. See issue emberjs#13284.
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

3 participants