Svelte: unrevert RepoPopover

[camdencheek]

This re-applies https://github.com/sourcegraph/sourcegraph/pull/61989 after it was reverted.

More detail inline, but in short:

• It reverts the changes to Popover.svelte that removed the border.
• We only start loading data on hover, not on mount
• Various fixes in text overflow conditions
• Removes the language from the popover data because it can be very expensive to calculate (another reason to pre-calculate language, but that's for another day)
• Moves the data loading out of the page loader. Exports the data loading function from the component so data loading is still orchestrated by the caller. (I know this will be controversial, reasoning inline)
• Adds a delay to the popover so it doesn't get in the way as your mouse moves over the page.
• Uses the display name instead of the author name
• Linkifies the commit message

Test plan

Storybook and lots of manual testing. Now that I'm writing this, we should probably add a playwright test too.

[camdencheek]

Small addition that ensures that the contents of the popover do not overflow the popover. Notably, this ensures that the border radius gets applied to any children as well (otherwise the corners of square children will overlap the border)

[camdencheek]

I removed language because this is calculated with inventory, which is stupidly expensive and flaky on large repos. This was regularly timing out for repos like llvm.

As a more general conversation point: I do not think we should be implementing any features at this point that do not scale to large customers.

[camdencheek]

I know this is controversial, but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I think this strikes a good balance by colocating the definition of the data fetcher with the component that uses it, but not fetching the data on mount, which allows the caller to handle data fetching however it wants. In this case, we do not want to fetch the data on page load, and we also want to add a delay before rendering. That would not be possible if we fetched the data on mount inside the component.

[fkling]

but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I suspected that this would happen eventually, and was hoping that when that time comes we know how we want to approach this.
I actually thought about this recently. I think there is kind of a forth type of data: on demand data that is unrelated (or only loosely related) to the current URL. For this kind of data it doesn't make sense to use the data loader. We will never preload the data and it's likely that this data (components) is needed (used) on multiple pages.
I arrived at a similar solution: These components would define a "loader" that handles the data loading for them and they would get passed an instance of the loader. This way we can still easily test the component and we are separating concerns. This was with more complex data loading in mind, e.g. infinity scroll.
In a case like this it seems fine to just have a function returning the results and pass those to the component.

So yeah, I think this approach is perfectly fine and we should use similar patterns for similar components.

Side note: The whole "only fetch data in data loader" principle was never meant to be absolute. It should help us understand what kind of data we are working with and determine appropriate exceptions to this rule.

[camdencheek]

This all makes sense to me! Thanks for adding some color 🙂

[camdencheek]

In the component itself, I made a number of DOM and CSS simplifications, most of which did not affect rendering, a few of which fixed rendering for weird edge cases like empty description or long commit message.

[camdencheek]

Small utility to delay resolution of a promise. We may want to add this as a general option on the Popover component, but I wanted to see how it feels first.

[fkling]

Thank you!

[fkling]

but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I suspected that this would happen eventually, and was hoping that when that time comes we know how we want to approach this.
I actually thought about this recently. I think there is kind of a forth type of data: on demand data that is unrelated (or only loosely related) to the current URL. For this kind of data it doesn't make sense to use the data loader. We will never preload the data and it's likely that this data (components) is needed (used) on multiple pages.
I arrived at a similar solution: These components would define a "loader" that handles the data loading for them and they would get passed an instance of the loader. This way we can still easily test the component and we are separating concerns. This was with more complex data loading in mind, e.g. infinity scroll.
In a case like this it seems fine to just have a function returning the results and pass those to the component.

So yeah, I think this approach is perfectly fine and we should use similar patterns for similar components.

Side note: The whole "only fetch data in data loader" principle was never meant to be absolute. It should help us understand what kind of data we are working with and determine appropriate exceptions to this rule.