Skip to content

Conversation

@samouri
Copy link
Contributor

@samouri samouri commented Aug 25, 2017

This PR seeks to add in the caterpillar + comment expansion behavior for reader conversations.

There are a number of challenges still:

  1. performance. in order to grab the right number of gravatars, comments, posts, etc to display everything along with the selectors etc. etc things can get hairy really quick if we dont use all the right data structures. the first stab I just finished definitely needs to be fixed in this regard, currently onload the tab stays at 100% cpu for far too long. I have a few theories on this. one of the first things i plan on addressing is the current brute force rendering strategy where its rendering every postcomment even if there is nothing to do for any of its children.that means that any inefficiencies in needless rerenders explode.
  2. comment nesting. the same logic currently on full-post won't work. we need the depth counting to begin at the first child that is rendered.
  3. displaying subcaterpillars.
  4. freshness / caching: we should start caching comments because it really helps with load perf of the page. we need to make sure it doesn't take up too much space as well as make sure that we will update for freshness every N mins on both full-post + convos
  5. styling. need to port over many of the style from current full-post

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Aug 25, 2017
@samouri samouri self-assigned this Aug 25, 2017
@samouri samouri added this to the Reader Conversations Tool milestone Aug 25, 2017
@samouri samouri added [Feature] Reader The reader site on Calypso. [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Aug 25, 2017
@samouri
Copy link
Contributor Author

samouri commented Aug 25, 2017

main perf issue is something that happens from the callback chain of data-layer's addComments

@samouri
Copy link
Contributor Author

samouri commented Aug 28, 2017

After a bunch of research it turns out the biggest perf issue was how the comments selectors were using createSelector (memoized selector creation). Turns out that createSelector was invalidating the cache almost every single call which ballooned.

@samouri
Copy link
Contributor Author

samouri commented Aug 28, 2017

I'm going to start breaking up this PR into many much smaller PRs. Its currently a mammoth

@samouri
Copy link
Contributor Author

samouri commented Aug 29, 2017

Theres a couple more parts to break out, but what i've broken up so far is:

  1. comment selector perf: Comments: invalidate createSelector cache based on state.comments.items instead of posts #17585
  2. expansions reducer: Reader Caterpillars: Expansions reducer + friends #17589
  3. comment replying: Conversations Tool: allow for replying to comments inline #17587
  4. id consistifying: Comments: make the proptypes for comments more consistent #17586
  5. format things! Comments: @format all of state/comments #17570

Things still to break out:

  1. caterpillar/postcomment connective tissue: Convos Caterpillar: connective tissue #17608
  2. watermark reducer

Also, we shouldn't implement the high watermark reducer the way done in this PR, it isn't generic enough.
I think we can create a reducer at state.reader.watermark that keys off stream identifiers so that instead of just applying to comments it can apply to any streams. That way it also mirrors the plan for the backend.

@samouri
Copy link
Contributor Author

samouri commented Aug 30, 2017

closing b/c #17608 covers everything here.
except for watermark but I'll open up another issue for that now

@samouri samouri closed this Aug 30, 2017
@samouri samouri deleted the add/reader-caterpillars/reduxwork branch August 30, 2017 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Reader The reader site on Calypso. [Size] XL Probably needs to be broken down into multiple smaller issues [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants