Closed mportiz08 closed 1 month ago
@swift-ci test
How we render the link on a page should not depend on the index data
On this note, is it possible for a link to be invalid in the navigator? Say if a symbol is curated in both modules, but we dropped the other module when hosting.
Originally, I was gonna say that we should double check and pass the value of the 2 new props everywhere there's potentially a broken link. Such as the
Card
component,Mention
component, andButtonLink
component. Having to make sure to do this everywhere we usereferences
is why I didn't like this method as much when I reviewed your original PR.I was curious as to how the
reduce
method could possibly cause things to slow down this drastically. I looked into this a bit and think the real problem is that we are re-sanitizing thereference
every time we use thereferenceProvider
mixin. That mixin is used in so many components, practically every link lol. It's probably doing O(n^2).
Thanks for pointing this out @hqhhuang. The real problem is not the original reduce
call on its own but the fact that it gets called way too many times (probably like c n and not n n where c is the number of unique components using the mixin). I forgot that even though the computed property is cached, it still gets created/cached for each unique component that it's in and not once for all components using the mixin.
With all that in mind, I'm probably going to scrap this PR in favor of another one where I do the original reduce logic but in a smarter place where it only should happen once. Even though this PR resolves the performance, I agree that doing the logic in this way is more complicated than the original method of updating the original reference data.
We should move that logic to before we set the sanitized
references
object to the store. Then, the provider can just provide without any additional computation. Idk where is a good place to do that though... There is no guarantee whether the index or page data gets fetched first. Do you think it's an option to have DocC emitincludedArchiveIdentifiers
in each page's metadata? I think that makes more sense anyways. How we render the link on a page should not depend on the index data. Is there a particular reason why it's inindex.json
?
I discussed this with @d-ronnqvist offline in the past, and it sounded like emitting this data in the Render JSON for individual pages (or doing this work in the compiler itself) would have been too expensive performance-wise and/or not possible (don't recall exactly) since it would involve rewriting already emitted data.
Opened a new PR for this: https://github.com/swiftlang/swift-docc-render/pull/900
Bug/issue #, if applicable: 136247329 (#894)
Summary
This PR re-implements #878 in a way that has a less noticeable performance impact on the initial page load experience. Instead of stripping out
url
data from the top-levelreferences
object, each individualReference
component will dynamically check for its "active-ness" as the index data is loaded.The advantage to this new approach is that it will have a smaller impact on the initial access of
references
, which can be fairly noticeable for pages with huge numbers of links. (See #894 for an example)One drawback to this new approach is that the links will start out as clickable links and quickly flash to non-linked text since this check will now happen dynamically when the index is loaded instead of before all references are available. I discussed this offline with @d-ronnqvist, and he believes that this is an acceptable tradeoff.
Testing
Steps:
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded