dart-lang / pub-dev

The pub.dev website
https://pub.dev
BSD 3-Clause "New" or "Revised" License
782 stars 147 forks source link

API docs on pub don't interlink between packages #7511

Open dcharkes opened 8 months ago

dcharkes commented 8 months ago

Consider https://pub.dev/documentation/native_assets_cli/latest/native_assets_cli/BuildConfig/BuildConfig.fromConfig.html, the Config type is from https://pub.dev/documentation/cli_config/latest/cli_config/Config-class.html.

Would it be possible to generate the API docs in such a way that types from other packages can resolve to the API docs of said package?

isoos commented 7 months ago

I believe this should happen automatically, we shall investigate why it is missing.

isoos commented 7 months ago

A similar and maybe smaller case in shelf_router, the Handler class from package:shelf is not referenced: https://pub.dev/documentation/shelf_router/latest/shelf_router/Router-class.html

I don't see any trivially missing parameter from here: https://github.com/dart-lang/pana/blob/master/lib/src/sdk_env.dart#L436-L445

/cc @srawlins @kallentu @parlough any idea what we are missing here?

jonasfj commented 7 months ago

We could add --link-to-remote, but it says (defaults to on).

But we probably need to reproduce locally.

sebouh00 commented 1 week ago

We're experiencing this with our packages. Was it addressed by any chance?

srawlins commented 1 week ago

Sorry, I don't believe it has been addressed. Can you give a repro, @sebouh00 ?

sebouh00 commented 1 week ago

For example, TransportEvent on this page references the type defined here.

srawlins commented 6 days ago

Curious, I'm wondering if it is a pub-specific problem. Or just flags, as @jonasfj suggests.

When I checkout the shelf repo, and run dart doc (using Dart 3.5.0) from the pkgs/shelf_router dir, I also see the Router constructor properly link to Handler. I don't see any dartdoc_options file in that repo.

When I run dartdoc's task.dart, I see the Router constructor (Router({Handler notFoundHandler = _defaultNotFound})) properly linking to Handler. That task runs dart --enable-asserts /Users/srawlins/code/dart-dartdoc/bin/dartdoc.dart --link-to-remote --show-progress --show-stats --no-validate-links. (Note the explicit --link-to-remote.)

When I take the command that dartdoc's task.dart runs, and remove the --link-to-remote, I still see the link to Handler. :/

isoos commented 6 days ago

@srawlins from the analysis logs, these are the currently used parameters for dartdoc on pub.dev:

2024-10-16 12:17:27.785044 INFO: Running `/home/worker/dart/stable/bin/dart pub global activate dartdoc 8.1.0`...
2024-10-16 12:17:37.677619 INFO: Running `/home/worker/dart/stable/bin/dart pub global run dartdoc --output /tmp/dartdoc-shelf_routerIYIMZW --sanitize-html --max-file-count 10000000 --max-total-size 2147483648 --no-validate-links --sdk-dir /home/worker/dart/stable`...
srawlins commented 6 days ago

I see. That also produces link to Handler:

$ dart pub global activate dartdoc
$ dart pub global run dartdoc \
    --sanitize-html \
    --max-file-count 10000000 \
    --max-total-size 2147483648 \
    --no-validate-links \
    --sdk-dir /Users/srawlins/code/dart-sdk/sdk/xcodebuild/ReleaseX64/dart-sdk/
$ less doc/api/shelf_router/Router-class.html
# or
$ dart pub global run dhttpd --port 9000 --path doc/api
jonasfj commented 5 days ago

@isoos could we be stripping the links during sanitization?

isoos commented 5 days ago

@isoos could we be stripping the links during sanitization?

Running this locally, I think we may do that. I just don't see why we would do it, as otherwise the markup is valid and correct.

jonasfj commented 5 days ago

Running this locally, I think we may do that. I just don't see why we would do it, as otherwise the markup is valid and correct.

@isoos I'm guessing we might be doing it because we want to remove relative links that go further up than / for the current package. So retry we might remove links that navigate up from /documentation/retry/latest/ OR maybe we just restrict links to other pages on pub.dev

My guess would be that we might do this because we're concerned of links being used for attacks. I'm not sure we should be. Probably not. Since we don't have any GET requests that trigger mutation.

isoos commented 5 days ago

This is the section where we remove the Handler link:

 <section class="summary offset-anchor" id="constructors">
    <h2>Constructors</h2>
    <dl class="constructor-summary-list">
        <dt id="Router" class="callable">
          <span class="name"><a href="../shelf_router/Router/Router.html">Router</a></span><span class="signature">({<span class="parameter" id="-param-notFoundHandler"><span class="type-annotation"><a href="https://pub.dev/documentation/shelf/1.4.2/shelf/Handler.html">Handler</a></span> <span class="parameter-name">notFoundHandler</span> = <span class="default-value">_defaultNotFound</span></span>})</span>
        </dt>
        <dd>
          Creates a new <a href="../shelf_router/Router-class.html">Router</a> routing requests to handlers.
        </dd>
    </dl>
  </section>

We shouldn't remove absolute links, and furthermore: I don't see where would do it for DartdocPage.content.