Inactivate known external links (Re-implementation)

[mportiz08]

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-level references object, each individual Reference 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:

1. Follow testing instructions from Inactivate known external links #878 and verify that the behavior there still works as expected.
2. Verify that the performance issue observed in Slow rendering of root-level pages #894 is mitigated.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran npm test, and it succeeded
• Updated documentation if necessary

[mportiz08]

@swift-ci test

[hqhhuang]

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, and ButtonLink component. Having to make sure to do this everywhere we use references 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 the reference every time we use the referenceProvider mixin. That mixin is used in so many components, practically every link lol. It's probably doing O(n^2).

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 emit includedArchiveIdentifiers 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 in index.json?

[hqhhuang]

fancy 👀

[hqhhuang]

Is this PR urgent? I moved includedArchiveIdentifier to the new IndexStore. When that lands, we will run into merge conflicts, we would probably need to keep includedArchiveIdentifier when we resolve the conflicts and open a new PR to move it to IndexStore. Would you prefer doing that over waiting a bit on this PR?

[hqhhuang]

(ignore this comment if we end up getting this metadata from each page's RenderJSON).

[hqhhuang]

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.

[mportiz08]

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, and ButtonLink component. Having to make sure to do this everywhere we use references 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 the reference every time we use the referenceProvider 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 emit includedArchiveIdentifiers 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 in index.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.

[mportiz08]

Opened a new PR for this: #900