-
Notifications
You must be signed in to change notification settings - Fork 125
Split sidebars out into separate HTML files #3384
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
Conversation
I think this came in when I was in the weeds of the 3.0 analytics push. I think this is great! It looks like we are having some problems with the tests though, maybe a sync / rebuild? |
CI is green 😁 |
@@ -99,6 +99,10 @@ abstract class TemplateData<T extends Documentable> extends TemplateDataBase { | |||
@override | |||
T get self; | |||
|
|||
String? get aboveSidebarPath; |
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.
Each template now has an optional aboveSidebarPath
and a belowSidebarPath
which is included in the main HTML, and the JS uses those paths to request each sidebar client-side.
} | ||
|
||
/// Template data for Mixin declarations. | ||
class MixinTemplateData extends InheritingContainerTemplateData<Mixin> { | ||
MixinTemplateData(super.htmlOptions, super.packageGraph, super.library, | ||
super.mixin, super.sidebarForLibrary, super.sidebarForContainer); | ||
MixinTemplateData( |
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.
These constructors no longer need to take sidebarForLibrary
and sidebarForContainer
, the previously computed-and-cached HTML blobs.
@@ -268,6 +268,15 @@ abstract class Container extends ModelElement | |||
@override | |||
String get filePath => '${library.dirName}/$fileName'; | |||
|
|||
/// The full path of this element's sidebar file. | |||
String get sidebarPath; |
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.
Containers have a notion of their own sidebar path (their "below" sidebar path), and then their child elements can call enclosingElement.sidebarPath
for their aboveSidebarPaths.
@@ -37,6 +37,52 @@ class HtmlGeneratorBackend extends GeneratorBackendBase { | |||
HtmlGeneratorBackend( | |||
super.options, super.templates, super.writer, super.resourceProvider); | |||
|
|||
@override | |||
void generateClass(PackageGraph packageGraph, Library library, Class clazz) { |
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.
Each of these which used to just render one HTML file must now render two, with the addition of their sidebar file, so they are overridden here, adding a lot of repeated boilerplate.
@@ -56,19 +56,6 @@ class DartdocGeneratorBackendOptions implements TemplateOptions { | |||
resourcesDir = context.resourcesDir; | |||
} | |||
|
|||
class SidebarGenerator<T extends TemplateData> { |
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.
We used to cache the sidebars generated, and no longer need to.
@@ -205,8 +184,7 @@ abstract class GeneratorBackendBase implements GeneratorBackend { | |||
|
|||
@override | |||
void generateClass(PackageGraph packageGraph, Library library, Class clazz) { | |||
var data = ClassTemplateData(options, packageGraph, library, clazz, |
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.
We no longer need to pass around these sidebar generators.
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 seems fine as long as any performance regression is not too severe. See my one comment.
void generateClass(PackageGraph packageGraph, Library library, Class clazz) { | ||
super.generateClass(packageGraph, library, clazz); | ||
var data = ClassTemplateData(options, packageGraph, library, clazz); | ||
var sidebarContent = templates.renderSidebarForContainer(data); |
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.
Doesn't this undo the performance benefits of caching the sidebar? We're going to be running the template for the sidebar repeatedly unless I'm misreading something.
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 shouldn't undo the caching benefits we had earlier (it should more-or-less mimic them): The problem we had before caching is that each page would independently generate it's left sidebar (sibling elements) and right sidebar (children), so that the left sidebar was being generated n+1 times for n siblings (by each of the n siblings, and once by the parent). The caching mechanism ensured each sidebar was generated once. This change does something similar: each TemplateData object is responsible for generating its own right sidebar, containing is children elements.
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.
OK, I had been misreading the code then. LGTM
Some timing notes. I checked three cases, and ran multiple times. I used the "Documented in n seconds" text as the metric.
There is a huge deviation in the times to document flutter, and even a moderate deviation in the others. So anything under 5 or 10% is probably within the margin of error. I'm considering it a no-op for typical packages, and even large packages like flutter. It's an improvement for packages with massive numbers of constants and variables and functions relative to their containers. |
Looks like a resync/ |
Ok, 😮💨 I got CI back to green. I think this is good to go if you do. |
Yes, let's get this landed. I want to publish now that 3.0 is out, and it would be great to have this included. |
…c pages (#128442) This updates dartdoc to 6.3.0. Release notes are available, here: https://github.com/dart-lang/dartdoc/releases/tag/v6.3.0 Most important for Flutter are the reduction in the size of generated HTML files (dart-lang/dartdoc#3384) and a new dartdoc directive to hide constant implementations from indicated classes (dart-lang/dartdoc#3398), which fixes the longstanding issue (dart-lang/dartdoc#2657). I've also added the api documentation zip to `.gitignore` and the `{@hideConstantImplementations}` dartdoc directive to the motivating example. A screenshot:  I assert that this change to icons.dart should be test-exempt as existing tests cover whether or not dartdoc directives are recognized or are leaking into HTML, and the impact of adding the directive was tested in dart-lang/dartdoc#3398.
This takes us in the direction of a Single Page App. The idea is to solve the output size issues (#3311, #2995, ...) by not generating duplicate sidebars everywhere.
Instead we can generate the sidebars as separate pages, and load them dynamically. This PR implements #3079.
Since we do not have functional tests for the generated output (web tests), I'll verify that I manually tested these things:
PACKAGE_NAME=collection PACKAGE_VERSION=1.17.1 dart run grinder serve-pub-package
<body data-base-href="../" data-using-base-href="false">
.<body data-base-href="../" data-using-base-href="false">
.dart run grinder serve-sdk-docs
<body data-base-href="../" data-using-base-href="true">
.