-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add support for custom scripts #1056
base: main
Are you sure you want to change the base?
Conversation
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.
This is making an unintentional source breaking change that should be undone.
Additionally, we're in the process of migrating away from LocalFileSystemDataProvider
so the new code to find the custom-scripts.json file also need to be added to DocumentationContext.InputProvider
. There's already a test that verifies that both implementation discover the same files which would also be good to update so that it verifies the behavior for this new type of file.
Sources/SwiftDocC/Infrastructure/Workspace/LocalFileSystemDataProvider+BundleDiscovery.swift
Outdated
Show resolved
Hide resolved
@@ -120,6 +120,18 @@ struct ConvertFileWritingConsumer: ConvertOutputConsumer { | |||
for downloadAsset in context.registeredDownloadsAssets(forBundleID: bundleIdentifier) { | |||
try copyAsset(downloadAsset, to: downloadsDirectory) | |||
} | |||
|
|||
// Create custom scripts directory if needed. Do not append the bundle identifier. |
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.
Why not append the bundle ID here?
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.
- So that projects with combined documentation of multiple targets (when that lands) can share scripts between the documentation bundles of each target. I suspect that authors of packages with multiple targets will want scripts to be shared: nobody wants to set up their MathJax scripts once per target.
- In practice, even in documentation webpages for multiple targets, custom scripts loaded into the browser are global. For example, scripts that define the same global function can conflict with each other irrespective of the bundle they were loaded from. So namespacing the script files by bundle ID is moot.
(Also, if the relative path to the custom-scripts
folder is always "/custom-scripts"
then it’s easier to find it in the renderer code.)
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.
I see. That's not how combined documentation works (which is already available as an experimental feature). Non-content files aren't automatically included in the combined archive. If this was treated as an asset, a collision between archives would be treated as an error. If this file is intended to be copied over into the combined archive, it should have an archive-unique prefix and this PR should update the merge command to copy over the expected files.
private func test( | ||
whether predicate: (URL) -> Bool, | ||
matchesFilesNamed fileName: String, | ||
withExtension extension: String | ||
) { |
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.
minor: A test helper like this which doesn't forward the callers line and file information would attribute a test failure to the wrong line. This is fixed by both:
- adding arguments that default to
#filePath
and#line
- explicitly passing those values to each test assertion in this helper.
Also, the "test" prefix is typically used for tests themselves, not test helpers.
private func test( | |
whether predicate: (URL) -> Bool, | |
matchesFilesNamed fileName: String, | |
withExtension extension: String | |
) { | |
private func assertThat( | |
_ predicate: (URL) -> Bool, | |
matchesFilesNamed fileName: String, | |
withExtension extension: String, | |
file: StaticString = #filePath, | |
line: UInt = #line | |
) { |
TextFile(name: "footer.html", utf8Content: ""), | ||
TextFile(name: "theme-settings.json", utf8Content: ""), | ||
TextFile(name: "custom-scripts.json", utf8Content: ""), |
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.
This added file is never verified anywhere. Other similar files are verified here.
Summary
First-class support for adding custom scripts to a DocC-generated website. These may be local scripts, in which case the website will continue to work offline, or they may be external scripts at a user-specified URL. This support is in the form of a custom-scripts.json file, the scripting analog of theme-settings.json.
Full pitch: https://forums.swift.org/t/pitch-support-for-custom-scripts-in-docc/75357.
Dependencies
Corresponding change in swift-docc-render.
Testing
custom-scripts.json
and the custommathjax-config.js
script (shown in the pitch) to your documentation catalog.docc convert
the documentation catalog with your custom scripts. Observe that the modified swift-docc copiedcustom-scripts.json
and the custom scripts into the documentation archive.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded