search

code-corps / code-corps-api

Elixir/Phoenix API for Code Corps.
https://www.codecorps.org
MIT License
235 stars 86 forks source link

Include conversation parts in conversation #1349

Open snewcomer opened 6 years ago

snewcomer commented 6 years ago

This results in significantly faster UI and smoother experience. Lmk what you think!

snewcomer commented 6 years ago

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.

joshsmith commented 6 years ago

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.

begedin commented 6 years ago

@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

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 commented 6 years ago

@begedin is this PR g2 merge?

begedin commented 6 years ago

@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.