Remove <base href /> and relative hrefs #2090

jdkoren · 2019-12-12T19:10:04Z

Context

Currently DartDoc emits relative hrefs when linking to another generated page. These hrefs all start from the docs output directory. In order to make these work, each page also includes a <base href /> in the <head> that walks back to the docs output directory (i.e. .. or ../..). For example, consider these sources:

library a;

class Foo {
  void hello(Bar bar) { /* ... */ }
}

library b;

class Bar { /* ... */ }

The docs tree would partially look like this:

api/docs/
├── a/
│   ├── a-library.html
│   ├── Foo/
│   │   └── hello-method.class
│   └── Foo-class.html
└── b/
    ├── b-library.html
    └── Bar-class.html

In a/Foo-class.html DartDoc emits <base href=".." />, and emits links to Bar in the form of <a href="b/Bar-class.html" />. Such links will be resolved properly as
{api/docs}/a/../b/Bar-class.html => {api/docs}/b/Bar-class.html.
Without the <base href>, the link would go to a nonexistent page: {api/docs}/a/b/Bar-class.html.

While this technique does make the pages quite portable, we can't use it for Markdown output (#1479). Also, there may be hosting situations where this <base href /> isn't ideal or can't be used.

Proposal: absolute hrefs from site root

The current hrefs are already relative to the site root and can be made absolute by placing a / in front: <a href="/b/Bar-class.html" />. Assuming the site root is in fact api/docs/ in our example, these links will always resolve properly. #2089 implements this.

Caveats and concerns

Specifying a site root

Using a different site root requires updating all the generated hrefs. DartDoc could support a site root prefix to hrefs via a new DartdocOption, otherwise clients are left to using a separate process to find and replace the hrefs. This option is under consideration and not yet implemented.

Link validation

DartDoc tries to ensure that generated links lead to valid pages. This process needs to account for the new href format and any new options for a site root prefix.

Transitioning important DartDoc usages

Major users of DartDoc include api.dartlang.org and flutter.dev/api. These and other usages need to be transitioned smoothly.

The text was updated successfully, but these errors were encountered:

jcollins-g · 2019-12-12T20:31:30Z

Tagging @kwalrath for angular, @gspencergoog for Flutter.

jcollins-g · 2019-12-12T20:31:48Z

Tagging @isoos for pub.dev.

isoos · 2019-12-12T21:04:57Z

/cc @jonasfj @sigurdm

sigurdm · 2019-12-13T07:56:46Z

While this technique does make the pages quite portable, we can't use it for Markdown output

Just for clarity, could someone spell out for me why markdown does not support relative links?

pub.dev serves docs from something like: https://pub.dev/documentation/flutter_redux/latest/ I think that would work poorly with absolute links.

The current hrefs are already relative to the site root and can be made absolute by placing a / in front

Would specifying the empty site-root-prefix be the same as having relative links? In that way we can support both styles.

I think it would be great if we went a bit further with the configurability and made a mapping of host prefixes per package. Then the docs for one package could link to the docs for another. And we could perhaps even link to api.dart.dev for the standard libraries from https://pub.dev/documentation/ .
If you look at eg. https://pub.dev/documentation/retry/latest/retry/retry.html it would be nice to have links to Duration, double etc.

isoos · 2019-12-13T08:20:48Z

And we could perhaps even link to api.dart.dev for the standard libraries from

This used to work, but changed over time, and we have pending bugs to track it:
dart-lang/pub-dev#2108
dart-lang/pub-dev#2117

I had an attempt to fix it a few months back, however, I'm stuck with that, don't really understand what's going on there:
#1972

@jcollins-g: I think this is a separate issue, but it would be really great if you had the time to help us out here too.

jcollins-g · 2019-12-13T16:16:34Z

@isoos I really would like to help but unfortunately all of my time right now is committed to the NNBD migration tooling effort. I can answer questions and review code, but that's about it. I'll take a look at the above referenced bugs and answer what I can; feel free to hit me up on chat as well if you're in the thick of it.

jdkoren · 2019-12-13T16:55:32Z

Just for clarity, could someone spell out for me why markdown does not support relative links?

Relative links do work in Markdown. The part that doesn't work in Markdown is the <base> tag, which goes inside <html><head>...</head></html>, and at this moment Dartdoc relies on that tag to make the relative links resolve properly.

In the example above, multiple pages will link to the Bar class, and here is a table of what the relative links to be:

page	relative link to Bar
`a/Foo-class.html`	`href="../b/Bar-class.html"`
`a/Foo/hello-method.html`	`href="../../b/Bar-class.html"`
`b/b-library.html`	`href="Bar-class.html"` (or `href="../b/Bar-class.html"`)

In reality, Dartdoc never emits any of the links in the second column for Bar, it always emits href="b/Bar-class.html", which would only resolve correctly if you are at the site root (b's parent directory). The difference is handled by the <base> tag in each page, but that tag is not available to us in a Markdown page. Even for HTML, we have a use case which does not permit the use of the <base> tag.

Making Dartdoc emit complete relative links like those in the second column is far too complicated a change, unfortunately.

pub.dev serves docs from something like: https://pub.dev/documentation/flutter_redux/latest/ I think that would work poorly with absolute links

This is something we'd like to test out. It might be that you'd only supply documentation/flutter_redux/latest/ as the prefix.

Would specifying the empty site-root-prefix be the same as having relative links? In that way we can support both styles.

At the moment the goal is to remove relative links entirely for the reasons mentioned. Even with an empty prefix, I would still be prepending a / to make them absolute. (The prefix would go in front of the /.)

Supporting both styles feels like it would be a maintenance burden, but if that ends up being the only path that works, we can do it.

isoos · 2019-12-13T18:03:08Z

@jdkoren

Making Dartdoc emit complete relative links like those in the second column is far too complicated a change, unfortunately.

We had a similar issue with relative links coming from markdown contents, where a relative link inside the markdown is rewritten with an absolute base URL. We ended up writing a markdown NodeVisitor that rewrites the URLs:

I believe a similar approach may work for dartdoc too, as the relative (../../b/Bar-class.html) could be computed from the current file's path and the linked file's path.

I have always found the <base> tag weird, and I think making the URLs relative should be the way forward.

jdkoren · 2019-12-13T23:58:59Z

I believe a similar approach may work for dartdoc too, as the relative (../../b/Bar-class.html) could be computed from the current file's path and the linked file's path.

Emitting complete relative links would be fantastic, but I don't know if that is feasible with Dartdoc's current structure. Everything that accesses a model's href for the purpose of creating a link instead needs to computeRelativeLink(String from) instead. There are two big hurdles there:

The template library (mustache) is very limited. It binds values like {{ href }} using simple string replacement, but knows nothing about what it is binding. It cannot call functions or register helper functions to transform values given some additional context, like the current file's path.
Dart has many codepaths that trickle down to accessing href in order to compose raw HTML for mustache to consume (because doing that with mustache is far too complex; an example is linkedParams). All such codepaths would need to change, and that is a lot.

I'm very open to ideas here, but that's my current assessment.

sigurdm · 2019-12-16T07:05:33Z

Thanks for the explanation, I was unaware of how <base> tags work.

It might be that you'd only supply documentation/flutter_redux/latest/ as the prefix.

But when there is a newer version available we like to serve the old one like this: https://pub.dev/documentation/flutter_redux/0.5.2/ it would need changing all the links?

I'm very open to ideas here,

Without knowing the subtleties of dartdoc:
Could you

Change the href getter to be a function, taking the base url to be relative to? Then dartanalyzer would highlight everywhere you need to change (probably the same number of places as if you wanted to make links absolute).
Everywhere you render mustache pass in the relative links already precomputed using the above function. or alternatively pass in the base filename in the context object, and use a lambda to compute it.

I'm not saying it is easy - but it sounds feasible to me.

jcollins-g · 2019-12-16T17:19:17Z

Everywhere you render mustache pass in the relative links already precomputed using the above function. or alternatively pass in the base filename in the context object, and use a lambda to compute it.

I'm not saying it is easy - but it sounds feasible to me.

This is dipping into a lot technical debt in dartdoc and even in my discussions with @jdkoren I feel like we've handwaved a bit why this isn't easy -- maybe it's time to dive in and explore what's needed.

I think the key problem making this hard is the use of reflection in mustache template processing. Mustache has the ability to restrict template evaluation to a specified interface in your template classes (in fact, that's what the example does). You can then use lambdas as @sigurdm suggests.

But dartdoc uses the alternative (and poorer performing) evaluation that uses dart:mirrors to dynamically evaluate complex expressions inside the template to point to Dart objects. This creates somewhat magical evaluations of very simple Dart-like references e.g.:

  <div id="dartdoc-sidebar-left" class="col-xs-6 col-sm-3 col-md-2 sidebar sidebar-offcanvas-left">
    {{>search_sidebar}}
    <h5>{{parent.name}} {{parent.kind}}</h5>
    {{>sidebar_for_library}}
  </div>

The parent evaluates to TemplateData.parent, but that returns something that isn't a TemplateData and name and kind directly access members. In addition, if something isn't found in the template data class by mustache, it will use reflection and try to call members of TemplateData.self. This is most often seen in the use of linkedName in templates, but there are probably other uses that are less easy to find.

My initial thought is the correct, though slow path through this is to refactor TemplateData and friends to minimize or stop using reflection. This would be a slog, but at least nowadays mustache4dart reliably reports when template references are unresolved and @jdkoren has already extracted a lot of the rendering logic out -- so this is less of a mountain to climb than it used to be. It seems likely that some interfaces and code might be extracted out of ModelElement to some mixin or interface shared between TemplateData and ModelElement to avoid a lot of duplication of hasThingIterable, thingIterable, canonicalThingIterable, etc.

If you do manage to achieve this, hrefs can be the old form for Dartdoc's internal processing, and as a last step before writing files we can convert to relative paths that work for HTML (or something else that works for Markdown).

While we can still consider alternatives, so far it seems everything is understandably trying to avoid doing this big chunk of work. If we can find a reasonable workaround that doesn't make the current situation worse for dartdoc or our downstream users I'm for it. Other reasonable alternatives might include ditching mustache entirely for another template system that's more in line with our needs, but that doesn't exactly sound like saving effort.

jdkoren · 2019-12-16T17:49:23Z

Adding to what @jcollins-g wrote, there are other properties like linkedName (accessed via reflection) which trickle down to accessing href. All of those will need to support taking the base url of the current context.

There is also the issue that some of the properties being accessed are computed once and the result is held by the model thereafter. Any of those containing hrefs can no longer be memo-ized this way, regardless of what we do with mustache. Taking the example above, suppose we add this documentation:

 class Foo {
+  /// Says hello with a [Bar].
   void hello(Bar bar) { /* ... */ }
 }

Documentation is normally converted to an HTML string only once, with [Bar] becoming <a href="b/Bar-class.html" /> in that string. Because multiple pages will render this link (a/Foo-class.html and a/Foo/hello-method.html) and they need different relative links to Bar, we won't be able to store that computation any more. Documentation is especially bad because it requires parsing Markdown, rendering HTML, resolving the hrefs, and processing special directives (@tool, etc).

If the hrefs remained absolute, we could avoid recomputing this and other properties.

jcollins-g · 2019-12-16T18:10:27Z

Another way to handle that would be to have the Documentation rendering to "HTML" actually render to mustache with some kind of reference for those dynamic bits. That'd enable dynamic substitution of hrefs without forcing rerenders of all the markdown (which indeed would be expensive).

jdkoren · 2019-12-16T23:29:02Z

I have a somewhat cheeky approach that lets us make complete relative paths without drastic changes to Dartdoc. The basica idea is, instead of Dartdoc emitting href="b/Bar-class.html" everywhere for Bar, instead it will emit href="%%HREFBASE%%b/Bar-class.html". After rendering the template, we call String.replaceAll('%%HREFBASE%%', base), where base is what we used to put in the <base> tag of the current template.

It adds a little extra step to the template generation process, but it's still Dartdoc doing all the work, and the result should be working hrefs, so current users shouldn't need a transition.

#2096 starts the ball rolling by decoupling hrefs and filenames.

This was referenced Dec 17, 2019

Decouple href and from a model's filename #2096

Merged

Remove <base href> and emit absolute hrefs instead #2089

Closed

Remove <base> tag, use hrefs prepended with base #2098

Merged

jcollins-g closed this as completed Jan 9, 2020

jcollins-g added the customer pub label Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove <base href /> and relative hrefs #2090

Remove <base href /> and relative hrefs #2090

jdkoren commented Dec 12, 2019 •

edited

Loading

jcollins-g commented Dec 12, 2019

jcollins-g commented Dec 12, 2019

isoos commented Dec 12, 2019

sigurdm commented Dec 13, 2019

isoos commented Dec 13, 2019

jcollins-g commented Dec 13, 2019

jdkoren commented Dec 13, 2019

isoos commented Dec 13, 2019

jdkoren commented Dec 13, 2019

sigurdm commented Dec 16, 2019 •

edited

Loading

jcollins-g commented Dec 16, 2019

jdkoren commented Dec 16, 2019

jcollins-g commented Dec 16, 2019

jdkoren commented Dec 16, 2019

Remove <base href /> and relative hrefs #2090

Remove <base href /> and relative hrefs #2090

Comments

jdkoren commented Dec 12, 2019 • edited Loading

Context

Proposal: absolute hrefs from site root

Caveats and concerns

Specifying a site root

Link validation

Transitioning important DartDoc usages

jcollins-g commented Dec 12, 2019

jcollins-g commented Dec 12, 2019

isoos commented Dec 12, 2019

sigurdm commented Dec 13, 2019

isoos commented Dec 13, 2019

jcollins-g commented Dec 13, 2019

jdkoren commented Dec 13, 2019

isoos commented Dec 13, 2019

jdkoren commented Dec 13, 2019

sigurdm commented Dec 16, 2019 • edited Loading

jcollins-g commented Dec 16, 2019

jdkoren commented Dec 16, 2019

jcollins-g commented Dec 16, 2019

jdkoren commented Dec 16, 2019

jdkoren commented Dec 12, 2019 •

edited

Loading

sigurdm commented Dec 16, 2019 •

edited

Loading