-
Notifications
You must be signed in to change notification settings - Fork 131
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
Treat external content as pre-rendered #806
Conversation
rdar://78718811
The cause of the regression was accidentally copying the local cache while mutating it.
@swift-ci please test |
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 |
...C/DocumentationService/Convert/Fallback Link Resolution/ConvertServiceFallbackResolver.swift
Outdated
Show resolved
Hide resolved
...C/DocumentationService/Convert/Fallback Link Resolution/ConvertServiceFallbackResolver.swift
Outdated
Show resolved
Hide resolved
...tDocC/Infrastructure/External Data/Deprecated/DocumentationContext+DeprecatedResolvers.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/GlobalExternalSymbolResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/GlobalExternalSymbolResolver.swift
Show resolved
Hide resolved
There was a problem hiding this 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.
...rastructure/External Data/Deprecated/OutOfProcessReferenceResolver+DeprecatedResolvers.swift
Show resolved
Hide resolved
...C/DocumentationService/Convert/Fallback Link Resolution/ConvertServiceFallbackResolver.swift
Outdated
Show resolved
Hide resolved
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":
|
@swift-ci please test |
@swift-ci please test |
# Conflicts: # Sources/SwiftDocC/Infrastructure/DocumentationConverter.swift # Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift
@swift-ci please test |
@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
@swift-ci please test |
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
@swift-ci please test |
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
andExternalSymbolResolver
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. LaterFallbackReferenceResolver
adopted the same pattern for filling in local content in a partial documentation build.From the start, this decision had a couple of drawbacks;
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
, andFallbackReferenceResolver
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, sinceDocumentationContext
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
becameExternalDocumentationSource
. It returnsLinkResolver.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 aurlForResolvedReference(_:)
API.ExternalSymbolResolver
becauseGlobalExternalSymbolResolver
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
andFallbackAssetResolver
are combined into a single internalConvertServiceFallbackResolver
. 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 singleConvertServiceFallbackResolver
. 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
Instead of adding a
externalSymbolIndex
(in addition to the existingexternalCache
), 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 createdDocumentationContext/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 contentAfter all the tests passed, I updated
DocumentationContext
to leverage the sameContentCache
type for both local and external content. Doing so designs away some possible scenarios where a local value is updated in thedocumentationCache
but not thesymbolIndex
or vice versa. This also found a handful of places where thesymbolIndex
was passed asinout
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>
linksUpdate the test resolver to return URLs to some images on the web for both "card" and "icon" type images.
Configure DocC to use the test resolver to resolve external links
In any project with symbols, add an article with external links in prose, curation, and
@Links
directivesPreview documentation for that project and navigate to the added article
@Links
groups display the card page image.Navigate to a symbol that conforms to an external protocol (for example
Equatable
) or subclasses an external class (for exampleNSObject
)Navigate to a symbol with a declaration that mentions an external type (for example an
Int
property, or aString
argument, or aBool
return type)Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded