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

Clear cells and cellMap when items changes #110

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

paddyobrien
Copy link
Contributor

I ran into an issue with ember-collection where indexes are not correctly set when the items array changes to a new array which is a subset of the previous array. The use case was providing a filterable list of items to ember collection.

It seems the item indexes are cached on the cell and never updated. I've added calls in updateItems to clear _cells and _cellMap whenever items changes. I think this makes sense but I might be missing some broader performance considerations.

@raytiley
Copy link
Contributor

Sorry for not replying sooner, been busy with work and prepping to emberconf. I'm hoping to hack on ember-collection a bit this week and can dig into this more than, but II think you can just set the cells index here to itemIndex.

https://github.com/emberjs/ember-collection/blob/master/addon/components/ember-collection.js#L154-L169

@paddyobrien
Copy link
Contributor Author

Yep looks like that fixes the problem too. Thanks for looking at this. I'm at emberconf this week I'd love to chat to you about how exactly why this works if you have some spare time. I find that updateCells method quite complex and pretty hard to grok, I'd love to understand it a bit better and help decompose it.

@raytiley
Copy link
Contributor

@paddyobrien sure thing... I'll do my best to explain things, it's actually not tooo bad, just needs a bit of commenting.

Find me at emberconf, I'll be giving my talk end of day 1, so I might be less distracted day 2.

@paddyobrien
Copy link
Contributor Author

Any thoughts on merging this? I'm 90% sure the test failure is a false negative but I don't have permissions to kick it off again.

@raytiley
Copy link
Contributor

raytiley commented Apr 7, 2016

not ok 183 PhantomJS 1.9 - Browser "phantomjs /home/ubuntu/ember-collection/node_modules/ember-cli/node_modules/testem/assets/phantom.js http://localhost:7357/22/tests/index.html?hidepassed" exited unexpectedly with exit code null - I'll try to dig into this failure this week.

@v3ss0n
Copy link

v3ss0n commented Jun 10, 2016

This problem still exists?

@paddyobrien
Copy link
Contributor Author

@raytiley I re-poked this to build again and it looks like it's passing now. I suspect the failure was a result of something being broken in the beta channel.

@carlesnunez
Copy link

Merge that pr please, it works pretty good

@AdriaRios
Copy link

+1

@GavinJoyce
Copy link

@raytiley is there anything else that needs to be done here?

@balos1
Copy link

balos1 commented May 12, 2017

+1

@pboutin
Copy link

pboutin commented Sep 14, 2017

Any reason why this has not merged yet ?

@raytiley
Copy link
Contributor

Hey Everyone... Sorry for not really having a hand on the wheel here. I'll try to poke some people and see what the long term fate of this project is. Honestly I think that most of the early contributors don't have any itches to scratch with ember-collection at the moment, hence the radio silence from us.

@raytiley raytiley merged commit 1a31c67 into adopted-ember-addons:master Sep 14, 2017
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.

8 participants