code-corps
Include conversation parts in conversation
This results in significantly faster UI and smoother experience. Lmk what you think!
Just realized this would be terrible for perf on the index route. What we should do is perhaps make a diff view for the show route. Added when_included for conversation_parts.
Although this is a small (and correct) change, I'd like some thoughts from @begedin on this so we can figure out how we generally want to approach these types of performance changes, including our "standards" for how preloads work, etc.
@joshsmith
The "include them on show, but do not on index" is probably the closest to ideal we can get with implicit includes.
As I said already, ideally, the client would be able to specify when to include and when not to include, but outside of or until we have that, I'm not sure we can get better than this.
Note that the spec does support explicitly specified includes as optional behavior:
http://jsonapi.org/format/#fetching-includes
Based on the spec we could write a catch-all "filter" to preload and add includes. It could parse the comma-separated include param value and apply preloads automatically, with the fallback controller handling an invalid preload with a 400 response as per specification.
My concerns are
- can we simulate this in Mirage? If not, our acceptance tests lose a degree of certainty
- do all ember paths right now work correctly with the change? looking at a conversation, replying, etc.?
If the current behavior (item 2 in concerns) works, then my vote is to merge this and devote some time on implementing explicit include behavior sooner rather than later, since we do not actually want to lose the certainty in acceptance tests.
@snewcomer @joshsmith and I didn't get around to talking specifics yet - we may modify the approach a bit, but the work here is definitely useful and will be either merged fully as is, or in a modified form. Sorry to keep you waiting, but all of this is very much appreciated.