Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat external content as pre-rendered #806

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Jan 19, 2024

Bug/issue #, if applicable: rdar://78718811 #468

Summary

This cleans up a long standing design flaw of mixing local and external content.

History

Long before the initial open source release, we designed ExternalReferenceResolver and ExternalSymbolResolver as two facets of a mechanism to interface with other documentation sources. At the time, we thought that it would be a good idea to model the external pages the same as local pages so that all DocC's processing of local content would automatically apply to external content as well. Later FallbackReferenceResolver adopted the same pattern for filling in local content in a partial documentation build.

From the start, this decision had a couple of drawbacks;

  • Since the documentation context mixed local and external nodes, we needed to track which nodes were local and external to avoid rendering full pages for the external nodes. Also, the local context needed a mechanism for determining the URLs of the external content.
  • Since the documentation node models are initialized with the unprocessed markup, and then processed by DocC. Any external content needed to be turned back into markup to then be processed and rendered. This wasn't an issue for basic text formatting, but if the external content contained links, those links would need to be re-resolved in the local context where they most likely would fail to resolve.

Over time we added more features, some of which didn't work well with this design decision. The primary example is the support for page images. For external documentation sources to replicate the behavior of page images, the markup needed to reference the images by name and those images needed to be mixed with the other local assets from the local documentation catalog so that the images could be found when processing the markup and rendering the external content.

More recently, #710 added support for external content from other DocC documentation sources. This external content wasn't mixed with the local content and was modeled as already-rendered, so that DocC doesn't need to process or render it.

A guide through these changes

To address this design flaw, this PR makes a handful of changes, one leading to the other

New protocols

First, since ExternalReferenceResolver, ExternalSymbolResolver, and FallbackReferenceResolver are public protocols, it would be source breaking to add or remove functions to any of these protocols. In order to maintain source compatibility these protocols all needed to be deprecated with new protocols taking their place. Also, since DocumentationContext had public properties that held values conforming to these protocols, those properties also needed to be deprecated and replaced with new properties.

  • Looking for another name, ExternalReferenceResolver became ExternalDocumentationSource. It returns LinkResolver.ExternalEntity (same as in Support resolving documentation links to content in other DocC archives #710) and is no longer a throwing function because it is passed a reference it already successfully resolved. Because the local context no longer decides the URLs for the external content, there is no need for a urlForResolvedReference(_:) API.

  • ExternalSymbolResolver because GlobalExternalSymbolResolver since there could only ever be one symbol resolver for all external symbols references. Because external symbols are resolved using the symbol's precise identifier and later tracked with a reference, this protocol only has one function that returns both the external entity and its reference.

  • _ExternalAssetResolver only existed to workaround the issues with external page images and has no replacement. It is simply deprecated.

  • FallbackReferenceResolver and FallbackAssetResolver are combined into a single internal ConvertServiceFallbackResolver. These protocols were only ever intended to be used with the convert service as a mechanism for on-demand resolving references to local content in partial documentation builds. Because there's only one local bundle there's also only a single ConvertServiceFallbackResolver. Its API very much resembles the external documentation source's API with the added capability of resolving local assets. Even though the resolved entities represents "local" content, they shouldn't be processed and rendered in the partial build so for all intents and purposes they are external to the provided build inputs.

Tracking external symbols

Because rdar://116085974—which is a follow up to #710 for supporting external DocC symbols—isn't implemented yet. This PR needed to track the symbols resolved by GlobalExternalSymbolResolver and make sure that their references are associated with the conformances and declaration tokens of the local symbols that have some relationship to these external symbols.

For local symbols, this was tracked with two separate variables

var symbolIndex: [String /* symbol ID */ : ResolvedTopicReference]
var documentationCache: [ResolvedTopicReference: DocumentationNode]

Instead of adding a externalSymbolIndex (in addition to the existing externalCache), I identified that we need to track local and external content much the same way, but store different values. To abstract away the common behaviors I created DocumentationContext/ContentCache<Value> and used it to track the external content.

Various places that looked for the reference for a symbol ID needed to be updated to check for both local and external symbols.

Merging fallback link results

DocC gathers all external links and resolves them in a dedicated phase, but the fallback resolver resolves local links as part of the local link resolution. This required some changes to how the fallback results are gathered but gave a good single point to add fetch all the assets referenced by fallback entities and associate the data assets with the external entities without mixing any fallback content with the local content.

Using ContentCache for local content

After all the tests passed, I updated DocumentationContext to leverage the same ContentCache type for both local and external content. Doing so designs away some possible scenarios where a local value is updated in the documentationCache but not the symbolIndex or vice versa. This also found a handful of places where the symbolIndex was passed as inout but didn't need to be.

Dependencies

None

Testing

This can be tested using the bin/test-data-external-resolver and some <doc://com.test.bundle/something> links

  • Update the test resolver to return URLs to some images on the web for both "card" and "icon" type images.

    "topicImages": [
      {
        "type": "card",
        "identifier": "some-external-card-image-identifier"
      },
      {
        "type": "icon",
        "identifier": "some-external-icon-image-identifier"
      }
    ],
    "references": [
      {
        "type": "image",
        "identifier": "some-external-card-image-identifier",
        "variants": [
          {
            "url": "https:\/\/SOME_PATH_TO_SOME_IMAGE_HERE.png",
            "traits": [
              "1x"
            ]
          }
        ]
      },
      {
        "type": "image",
        "identifier": "some-external-icon-image-identifier",
        "variants": [
          {
            "url": "https:\/\/SOME_PATH_TO_SOME_IMAGE_HERE.png",
            "traits": [
              "1x"
            ]
          }
        ]
      }
    ],
  • Configure DocC to use the test resolver to resolve external links

    export DOCC_LINK_RESOLVER_EXECUTABLE=$PWD/bin/test-data-external-resolver
  • In any project with symbols, add an article with external links in prose, curation, and @Links directives

    # Some article
    
    This page links to external documentation in a few different ways
    
    Here's the link in prose: <doc://com.test.bundle/something>
    
    @Links(visualStyle: compactGrid) {
       - <doc://com.test.bundle/something>
    }
    
    @Links(visualStyle: detailedGrid) {
       - <doc://com.test.bundle/something>
    }
    
    ## Topics
    
    - <doc://com.test.bundle/something>
    
  • Preview documentation for that project and navigate to the added article

    • Verify that the link in the prose displays the link with the resolved title as the link text.

      Note that clicking the link won't work since there isn't a real page to link to.

    • Verify that both @Links groups display the card page image.
    • Verify that the curation displays the icon page image.
  • Navigate to a symbol that conforms to an external protocol (for example Equatable) or subclasses an external class (for example NSObject)

    • Verify that the Conforms To or Inherits From sections contains links with the resolved title as the link text.

      Note that clicking the link won't work since there isn't a real page to link to.

  • Navigate to a symbol with a declaration that mentions an external type (for example an Int property, or a String argument, or a Bool return type)

    • Hover over the external type name token in the declaration and verify that the token becomes underlined to indicate that it's a link.

      Note that clicking the link won't work since there isn't a real page to link to.

Checklist

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

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@d-ronnqvist d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Jan 19, 2024
@d-ronnqvist d-ronnqvist linked an issue Jan 19, 2024 that may be closed by this pull request
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

Note that this doesn't yet solve the problem with formatter abstracts in external content from other sources, as described in #468. That would require OutOfProcessReferenceResolver/ResolvedInformation with LinkDestinationSummary which would have made this PR even bigger. I created #802 to track that as a follow up change.

Copy link
Contributor

@patshaughnessy patshaughnessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement! nice work 👍 I'll test some more tomorrow but in the meantime I have a few questions... mostly Swift language questions.

@sofiaromorales
Copy link
Member

I don't think I'm fully aware about what is considered an external symbol. I know about links to external references, but, in which case I have an external symbol or resource? when mixing doc archives?

@d-ronnqvist
Copy link
Contributor Author

I don't think I'm fully aware about what is considered an external symbol. I know about links to external references, but, in which case I have an external symbol or resource? when mixing doc archives?

Basically, if there's no symbol with that unique identifier in any of the symbol graph, then that's an external symbol.

There are two different types of "external symbols":

  • Symbols in other modules, for example if a local symbol conforms to Swift/Equatable or has a function that returns Swift/Int.
  • Symbols in the same module in a partial symbol graph file (as described in this comment)

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@mayaepps @daniel-grumberg @sofiaromorales @patshaughnessy Is there anything else I can do to make it easier to review these changes? I was hoping to open new PRs that build on what's in this PR in the next couple of weeks.

# Conflicts:
#	Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@patshaughnessy
Copy link
Contributor

After some more extensive testing with a large, realistic documentation project, we were able to verify that Render JSON produced by the code in this PR vs. the main branch is equivalent. I'd go ahead and merge this 👍

# Conflicts:
#	Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 29f830f into apple:main Feb 13, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the treat-external-content-as-prerendered branch February 13, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Code cleanup *without* user facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Treat external content as "pre-rendered"
5 participants