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

[CP] [Stable] Cherry-pick request for updating dartdoc to fix sidebar generation #54073

Closed
parlough opened this issue Nov 16, 2023 · 8 comments
Closed
Assignees
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve dart-cli-doc merge-to-stable

Comments

@parlough
Copy link
Member

parlough commented Nov 16, 2023

Commit(s) to merge

https://dart.googlesource.com/sdk/+/279f6d77aed890abfdd0d93341d8b94239dca610

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/336780

Issue Description

#54067

There are two primary issues:

  • As seen in the api.dart.dev 3.2.0 API docs (Example), the sidenavs are not inserted on non-class pages.
  • Even if the sidebars were added correctly, the links were incorrect in the sidebar if not using base-href, preventing from them adjusted by the JS that adds in the sidebar. (Broken links in generated "API reference" pub-dev#7058).

What is the fix

Update to the earliest dartdoc commit that includes the necessary fixes. 279f6d7 updates to dart-lang/dartdoc@8c10339 which includes all three fixes:

These fixes are however not isolated and are mixed with a few other minor changes in the range dart-lang/dartdoc@a3cfdc4...8c10339.

@srawlins outlined the other commits as:

As part of those 4 commits, support for the hideSdkText option was dropped, but it was already hidden and not exposed by the dart doc CLI.

Why cherry-pick

The 3.2 API docs will be Dart developer's view into the SDK's public APIs for the next several months. Having a missing sidebar makes navigation across the site quite difficult and less accessible, and will be especially jarring for users who expect it to be there.

Without these fixes, user's own docs generated with dart doc will also be affected. Pub updates dartdoc separately so it does not need this cherry-pick.

Risk

low/medium

Issue link(s)

#54067, dart-lang/pub-dev#7058

Extra Info

Flutter has been using a version with these commits/fixes to generate their main-api docs since flutter/flutter@997a94f.

@itsjustkevin
Copy link
Contributor

@mit-mit I see this as a P0 and would like to get it cherry-picked in.

CC: @MaryaBelanger

@vsmenon
Copy link
Member

vsmenon commented Nov 17, 2023

@srawlins - what is your take on benefit / risk?

@srawlins
Copy link
Member

I support this cherry-pick. Here are some details about the dartdoc commits being added:

I'll also note that Flutter bumped to dartdoc 7.0.1, for generation of api.flutter.dev, with flutter/flutter@997a94f, on Oct 20. They have not reported any regressions since running with that bump.

So this cherry-pick would contain 3 commits that fix the issue at hand, and 4 unrelated changes that may affect the output of a developer locally running dart doc.

Any unknown regressions would only affect the rendered docs at api.dart.dev (until we could land fixes), and output from a developer running dart doc locally; any unknown regressions would not affect docs at pub.dev or at api.flutter.dev. Therefore I think the risk is very very low of this cherry-pick causing anything more than a minor issue that could be corrected in a short time.

@vsmenon
Copy link
Member

vsmenon commented Nov 17, 2023

Thanks - lgtm

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Nov 17, 2023
@parlough
Copy link
Member Author

Thanks all!

I've updated the changelog in the CL as requested as well.

@sortie Does the release team submit the CL when ready or should I get someone else to submit it for me?

@sortie
Copy link
Contributor

sortie commented Nov 17, 2023

The normal workflow applies here once the cherry-pick-approved label is granted. Since you can't submit AFAIK, yeah, get someone to help you. I can do that for you if you want just let me know :)

@parlough
Copy link
Member Author

I can do that for you if you want just let me know :)

That would be great! Thank you 😄

copybara-service bot pushed a commit that referenced this issue Nov 17, 2023
Update dartdoc for missing sidebars and incorrect links in sidebars.

Fixes #54067
Closes #54073

Bug: #54067
Change-Id: I19c1e5c21a293aa29bfa6c1e93f097bba1fd8faa
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/330666
Cherry-pick-request: #54073
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336780
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Jonas Termansen <sortie@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
@mraleph mraleph added area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-doc labels Nov 20, 2023
@athomas athomas closed this as completed Nov 22, 2023
@athomas
Copy link
Member

athomas commented Nov 22, 2023

Released in 3.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve dart-cli-doc merge-to-stable
Projects
None yet
Development

No branches or pull requests

9 participants