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

Revert "Ember.warn when ManyArray.objectAt() is underfined" #4914

Merged
merged 1 commit into from
Apr 5, 2017

Conversation

stefanpenner
Copy link
Member

This reverts commit 44ffbff.

More details: #4896 (comment)

@stefanpenner
Copy link
Member Author

FYI: @runspired

@hjdivad hjdivad self-requested a review April 5, 2017 01:45
@runspired
Copy link
Contributor

I have thoughts, we may not totally want to revert, need to read up.

@runspired
Copy link
Contributor

Ok, read through the other comments and I agree this doesn't cover the unloaded case, and the sparse case is now handled separately. We should continue with the revert.

As a parting note: off of master our app was hitting this very often. This suggests to me the sparse case is not totally handled correctly yet; however, in #4882 I noticed that ManyArray was created eagerly at times we did not intend and fixed that. It may be that hitting the out of bounds check and the overly eager materialization of the ManyArray are the same problem.

@stefanpenner
Copy link
Member Author

stefanpenner commented Apr 5, 2017

@runspired ya, I would be curious in seeing the source of the out of bounds stuff you are seeing. I suspect test teardown or similar but further investigation there may yield some other underlying issue.

@stefanpenner stefanpenner merged commit 33f0ced into master Apr 5, 2017
@stefanpenner stefanpenner deleted the revert-4896 branch April 5, 2017 02:12
@stefanpenner stefanpenner restored the revert-4896 branch April 5, 2017 02:13
@stefanpenner stefanpenner deleted the revert-4896 branch April 5, 2017 02:13
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