Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Extract fragments from Accumulators and move them closer to the views that render them #2046

Open
smashwilson opened this issue Apr 8, 2019 · 1 comment

Comments

@smashwilson
Copy link
Contributor

Extracted from #1995 (comment):

Something to consider as a best practice -- since Relay is designed to have data co-located with the views that use them, I think we could extract sub-fragments from this graphql fragment and put them on the views that use them. Like reviewsView_comment and commentDecorationsController_comment.

[..]

We'd rather not fetch data we don't need, that's the beauty of GraphQL 😊

This also applies to the other two accumulators: ReviewSummariesAccumulator and ReviewThreadsAccumulator

@smashwilson
Copy link
Contributor Author

And copying my reply:

I started to try to address this and I hit a bit of a hitch with it. The aggregated data structure that we assemble in AggregatedReviewsContainer to hold the collated review threads and review comments is different from the way that the data is shaped in GraphQL, so we can't easily map it to props in a Relay fragmentContainer around ReviewsView.

Specifically, the GraphQL structure looks roughly like this:

... on PullRequest {
  reviewThreads {
    nodes {
      # fields we need from review threads
      ...reviewsView_reviewThreads
      comments {
        nodes {
          # fields we need from comments
          ...reviewsView_comments
        }
      }
    }
  }
}

We process comments grouped with their owning threads in ReviewsView like so:

const commentThreads = [
  {thread: {id: 'thread0'}, comments: [{id: 'comment0'}, {id: 'comment1'}]},
  {thread: {id: 'thread1'}, comments: [{id: 'comment2'}, {id: 'comment3'}]},
]

<ReviewsView commentThreads={commentThreads} />

But as our components are structured now, Relay would provide the fragment data to ReviewsView like this:

const reviewThreads = [
  {id: 'thread0'}, {id: 'thread1'},
]

const comments = [
  {id: 'comment0'}, {id: 'comment1'}, {id: 'comment2'}, {id: 'comment3'},
]

<ReviewsView reviewThreads={reviewThreads} comments={comments} />

So we'd lose the ability to tell which comment belongs to which review thread, which we use pretty heavily within the view.

I think one path forward to accomplish this is to split a subcomponent out of ReviewsView which can be a fragmentContainer for a single comment:

... on PullRequest {
  reviewThreads {
    nodes {
      # fields we need from review threads
      ...reviewsView_reviewThread
      comments {
        nodes {
          # fields we need from comments
          ...reviewCommentView_comment
        }
      }
    }
  }
}
class ReviewsView extends React.Component {
  renderReviewThread(thread) {
    // Relay will do its magic and allow each child to access each comment's data, but ReviewsView still gets to tell
    // it about its owning thread
    return thread.comments.map(comment => (
      <ReviewCommentView
        comment={comment}
        thread={thread}
      />
    ))
  }
}

In any case, this was a bit more invasive than I was hoping it would be, so I'm going to punt on it for the moment, and tackle this after we have a chance to get some of the easier ones out of the way, and maybe advocate merging as-is if we continue to run short on time. It's also possible that I'm misunderstanding some Relay-fu that would make this easier. I welcome education if so 😁

@smashwilson smashwilson mentioned this issue Apr 8, 2019
46 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant