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 release] Fix Ember.computed.sum #12170

Merged
merged 1 commit into from
Aug 22, 2015

Conversation

duggiefresh
Copy link
Contributor

Check to see if the dependentKey is an array, if not, then return
the initialValue.

Closes #12096

return get(this, dependentKey).reduce((previousValue, currentValue, index, array) => {
const arr = get(this, dependentKey);

if (!isArray(arr)) { return initialValue; }
Copy link
Member

Choose a reason for hiding this comment

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

I believe this check should actually be

(arr === null || arr === undefined)

If arr is an object and doesn't have reduce we should throw, but not if it is just not an array or an array like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 That makes sense. Should it throw if arr is a string?

Copy link
Member

Choose a reason for hiding this comment

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

If arr is an object and doesn't have reduce we should throw,

it should likely throw if it is not an array and not an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thanks for the feedback! Updated per review. :)

Check to see if the `dependentKey` is an array or object, if not, then return
the `initialValue`.

Closes emberjs#12096
rwjblue added a commit that referenced this pull request Aug 22, 2015
[BUGFIX release] Fix `Ember.computed.sum`
@rwjblue rwjblue merged commit 6272afa into emberjs:master Aug 22, 2015
@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

Thanks @duggiefresh!

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