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

[BUGFIX beta] Fix a recent regression with Ember.A #13287

Merged
merged 1 commit into from
Apr 9, 2016

Conversation

workmanw
Copy link

@workmanw workmanw commented Apr 9, 2016

Due to a recent regression, Ember.A(null) would return null instead of []. See issue #13284.

} else {
A = function (arr = []) {
if (arr === null) { arr = []; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the native version, you rely on truthiness as a check for arr, but in this version you look explicitly for null instead of !arr. Is there a reason for the difference?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixonic Yea that's a good point. I think I was just trying to be more explicit about what was happening. Someone reading the following might feel that it's redundant:

A = function (arr = []) {
  if (!arr) { arr = []; }
  return EmberArray.detect(arr) ? arr : NativeArray.apply(arr);
};

But it's definitely inconsistent between the two. So let's fix this, which do you think is prefered?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno- I would defer to the old behavior since this is a regression fix. Can you perhaps try it out on 2.4 or and older version and simply match that? Thanks man.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixonic Ha. It turns out the old behavior was inconsistent too. 586d4c6#diff-7. I think I'll just use the truthiness check and add a code comment. Then see what others think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭 truthiness was the mode with prototype extensions enabled (which is the most common), so I agree normalizing around that as a BUGFIX makes sense 👍

@@ -138,9 +138,11 @@ var A;

if (Ember.EXTEND_PROTOTYPES === true || Ember.EXTEND_PROTOTYPES.Array) {
NativeArray.apply(Array.prototype);
A = function (arr = []) { return arr; };
A = function (arr = []) { return arr || []; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why bother then with the default arg?

@krisselden
Copy link
Contributor

the default arg is redundant to handling an explicitly undefined or null, please remove.

@krisselden
Copy link
Contributor

also, the comment is unneeded with the regression test.

@krisselden
Copy link
Contributor

This regression is unreleased?

…)` returning `null` instead of an array. See issue emberjs#13284.
@workmanw
Copy link
Author

workmanw commented Apr 9, 2016

Chatted with @krisselden briefly on Slack. We agreed that keeping the default args was unnecessary in this case. As a result of that I removed the code comment as well.

This affects beta, it does not affect any release.

@krisselden krisselden changed the title Fix a recent regression with Ember.A [BUGFIX beta] Fix a recent regression with Ember.A Apr 9, 2016
@mixonic
Copy link
Member

mixonic commented Apr 9, 2016

Thanks @workmanw

@mixonic mixonic merged commit 6265067 into emberjs:master Apr 9, 2016
@workmanw
Copy link
Author

workmanw commented Apr 9, 2016

Anytime! 😄

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.

3 participants