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] Ensure ArrayMixin#invoke returns an Ember.A. #16785

Merged
merged 2 commits into from
Jun 28, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 28, 2018

This is a partial revert of bee179d, moving back to a manual forEach so that we can control the return value a bit more. When prototype extensions are disabled, we cannot rely on this.map to return an extended array.

Fixes a regression detected by ember-data's test suite running against ember-source's canary channel.

rwjblue added 2 commits June 28, 2018 12:07
This is a partial revert of bee179d, moving back to a manual `forEach`
so that we can control the return value a bit more. When prototype
extensions are disabled, we cannot rely on `this.map` to return an
extended array.
@rwjblue rwjblue mentioned this pull request Jun 28, 2018
@runspired
Copy link
Contributor

My only thought is that nothing outside of this one test in ember-data seems to have broken, and I haven't seen complaints from other folks (yet). The this.map behavior seems desirable. Do we maybe want to keep this fix in our pocket and just make ember-data not require the A for now?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 28, 2018

@runspired - The issue is that this.map for an Ember.A() doesn't always return an Ember.A() (when prototype extensions are disabled). Changing what invoke returns definitely seems like a breaking change to me.

Also, its unlikely to have affected many ember-data users yet because its only on canary, but I'm certain that folks are doing "special" things in their buildURL where they expect the snapshots to be an extended thing. Think:

buildURL(type, ids, snapshots) {
  let first = snapshots.get('firstObject'); // :boom: without this change

  // ...snip...
}

@rwjblue rwjblue merged commit c714f6e into emberjs:master Jun 28, 2018
@rwjblue rwjblue deleted the fix-invoke branch June 28, 2018 17:41
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.

2 participants