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

Include conversation parts in conversation #1349

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

snewcomer
Copy link
Contributor

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

@snewcomer
Copy link
Contributor Author

snewcomer commented Dec 28, 2017

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.

@snewcomer snewcomer force-pushed the include-convo-parts branch 3 times, most recently from 238986f to ffc82fc Compare December 28, 2017 15:14
"read-at" => conversation.read_at,
"status" => conversation.status,
"inserted-at" => conversation.inserted_at,
"updated-at" => conversation.updated_at
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to try to alpha order these so it's quicker to scan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today is the day I start alpha ordering always 😆

}
},
"included" => [%{
"attributes" => %{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably shift the object down and right one to match the style used earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@joshsmith
Copy link
Contributor

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
Copy link
Contributor

begedin commented Jan 2, 2018

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

@begedin begedin mentioned this pull request Jan 2, 2018
@snewcomer snewcomer force-pushed the include-convo-parts branch from d98b3a4 to 22322b8 Compare January 2, 2018 16:03
@snewcomer
Copy link
Contributor Author

@begedin is this PR g2 merge?

@begedin
Copy link
Contributor

begedin commented Jan 3, 2018

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

@begedin begedin force-pushed the develop branch 8 times, most recently from 4290c33 to 4f41648 Compare February 12, 2018 15:00
@begedin begedin force-pushed the develop branch 10 times, most recently from e075407 to 6d6cc63 Compare February 12, 2018 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants