search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Get higher_items and lower_items without extra database queries #280

Closed exchgr closed 5 years ago

exchgr commented 7 years ago

On some apps, database performance suffers quite a lot from use of this gem, since many method calls result in their own database queries. Here, I attempt to speed things up by using methods from Enumerable to get the same objects from acts_as_list_list, which we already have in memory. The difference is that these methods now return Arrays instead of ActiveRecord::Relations, so I've added more methods to get those relations back in cases where they're needed.

brendon commented 7 years ago

Hi @exchgr, thanks for doing this PR. I think I'll get a review done by @swanandp on this one as it's a bit of a deviation away from the norm for the gem. My concern would be stale data. There may be assumptions around the way things are currently done that aren't tested properly so it'd be a good idea to ensure it's all well tested.

brendon commented 7 years ago

Also, the test suite failed. Seems to be a trivial error though.

exchgr commented 7 years ago

Thanks for taking a look at this!

I could be wrong about this, but it seems that there wouldn't be stale data, unless it's stored in a cache somewhere that I missed. But also, I'm just using this in the context of rendering an action in Rails, which instantiates new ActiveRecord and SQL queries for each request and only caches repetitive ones for the duration of the request. Perhaps there are other use cases I overlooked. I'd love to discuss this more.

As far as I know, I only changed the implementation of these methods, and not (too drastically) the interface. From what I can tell, the only difference is that these methods now return Array instead of ActiveRecord::Relation. What else would need to be tested, if anything?

Re: the test failure, it looks like I just didn't account for the fact that this gem supports multiple versions of Rails, and I used Array#pluck, which isn't supported on some older versions. Should be an easy fix.

brendon commented 7 years ago

Thanks @exchgr, you make a good argument. I'd just like to wait for @swanandp to take a look too. He's pretty busy so it could be a while but he always gets there in the end :)

exchgr commented 7 years ago

Thanks for the update!

brendon commented 6 years ago

@exchgr, are you still wanting to progress this one? Looks like it needs some more development :)

brendon commented 5 years ago

Closing due to inactivity.