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

Flyout and Footer API design document #8052

Merged
merged 4 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
165 changes: 165 additions & 0 deletions docs/development/design/flyout-redesign.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
===============================
Flyout Redesign Design Document
===============================

.. warning::

This document details designs that are a **work in progress**.


This document describes the design of a new "flyout API"
to replace our existing :ref:`"footer_html" <api/v2:Undocumented resources and endpoints>` endpoint in APIv2.
The Read the Docs theme uses this API to put an updated version selector
with active versions into the lower left menu.
On other themes, this appears as a floating menu in the lower right.

.. contents:: Contents
:local:
:backlinks: none
:depth: 1


Issues with the existing API
------------------------------

.. figure:: ../../_static/images/design-docs/flyout/flyout-expanded.png
:align: right
:figwidth: 300px
:target: ../../_static/images/design-docs/flyout/flyout-expanded.png

Read the Docs with an expanded flyout menu in the lower left corner

* The largest problem with the existing ``/api/v2/footer_html`` endpoint
is that it returns a blob of HTML.
This limits the ability to use this data in ways other than to generate our exact flyout.
For example, Sphinx themes other than our own cannot consume this data easily
and there's no real integration point for MkDocs at all.
* Due to how the URLs in the version selector are generated,
this API endpoint can be fairly expensive in the worst case for projects with many versions.
As it stands now, this API accounts for approximately 15% of the time taken on our webservers
at about 70ms for an average request (P95 ~= 150ms, P99 ~= 235ms).
* The current API is a combination of data about a project that contains information
on the live versions, translations, sub- and super-projects, and links to various other things
like downloads, the repository on the VCS, and builds for the project on RTD itself.
Some of this data never changes (links to the project and builds on RTD itself)
and are effectively static while some of it could change after the documentation is built (translations, active versions).


Overlap with existing API endpoints
-----------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note here that we are using the proxied endpoint of the footer, this /_/api/v2/footer_html, this allow us to share the same domain with some advantages:

  • avoid being blocked by some extensions
  • Allow to cache the response (docs are served from cloudflare)
  • Allow to use the auth backends in .com

And, I've been having some ideas around our APIs... here is an extract from #8039

We should probably make a distinction between our general API that handles Read the Docs resources,
vs our APIs that expose features (like server side search, footer, and embed, all of them proxied).
This way we can version each endpoint separately.

I do agree that the footer shares a lot with the projects/ endpoint. Proxying api v3 may be a little of a problem, because it relies heavily on resolving URLs from the dashboard, but in proxito we don't have the url patterns from the main site.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good point. Maybe we don't need all the APIv3 endpoints but we can have an endpoint that just returns the same data as the APIv3 endpoint and shares code.


There is already significant overlap between the APIv2 ``footer_html`` call
with the existing APIv3 for a project (eg. ``https://readthedocs.org/api/v3/projects/docs/``).
The project API already returns much of the data we want,
but some things like other active versions, translations, and downloads would require additional API calls or options.
These options already partially implemented via the `DRF Rest Fields <https://pypi.org/project/drf-flex-fields/>`_ module
davidfischer marked this conversation as resolved.
Show resolved Hide resolved
(eg. ``https://readthedocs.org/api/v3/projects/docs/?expand=active_versions``).
Currently, there are separate API endpoints for translations and downloads,
but ideally all the data needed to generate a version selector would be available from a single API.
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we could implement ?expand=translations,subrpojects and we would have everything we need with one API call: https://readthedocs.org/api/v3/projects/docs/?expand=active_versions,translations,subprojects

One downside that has using this endpoint is that we will be returning much data than we need and that could slow down the response time.

Another thing to consider is that APIv3 is authenticated-only, and we require anonymous access for the flyout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sold on the idea of sharing the endpoint for the flyout than a regular endpoint. In the future we may want to add extra data to /api/v3/projects/ and we may be blocked because it increases the response time by some milliseconds that are not acceptable for the flyout endpoint.

I think that having an independent endpoint with more control over it is a win here and will avoid headaches in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed here. I think we want a dedicated API just for this use, but it could share code for sure. The footer API is large enough of a use-case to make it special.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also agree here, one flyout API seems like the best method. We don't yet have a good pattern for injecting an API access token into the build process, and this would be needed for all of the authenticated API endpoints.


While this is a good approach, it will not be without issues.
It's likely that some database queries and especially URL generation will need to be optimized
for this to not perform worse than the existing footer API.


JavaScript integration
----------------------

Our current footer API is requested from our embedded document JavaScript (``readthedocs-doc-embed.js``)
which is placed on pages during the build process.
Currently, this is a static file that offers no integration points for theme maintainers
or project authors.
There is also some static data about the project injected into the HTML of the generated documentation (``READTHEDOCS_DATA``)
which has some basic data about the project and the specific build being served.
This includes data such as the project and version slugs, the build date, and theme.

One of the goals of this project is to give theme authors and project maintainers integration points
where they can control some aspects of Read the Docs in a repeatable, deterministic way.
In order to do this, we need to have JavaScript constants and functions that these maintainers can set.
This proposal suggests creating a new ``readthedocs`` global variable like so:

.. code-block:: javascript

// This first line helps handle the case where the project/theme's overrides
// are executed before the readthedocs global is defined.
window.readthedocs = window.readthedocs || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One seemingly common use case that is not supported by this current proposal is extending the flyout display function/method. If a theme wants the flyout, but also wants to inject a version selector somewhere else in the UI, there is no way to extend this functionality, only override it.

That is, the user is expected to define a window.readthedocs.integration_version_selector to customize the display process, but there is no way to call the parent window.readthedocs.integration_version_selector function -- it isn't defined until after it is inspected by our client.

This could either be a reason to use a different pattern than defining overrides as global state, or we would need to require more granular override points for users.

window.readthedocs.data = JSON.parse(document.getElementById('READTHEDOCS_DATA').innerHTML);

if (!window.readthedocs.integration_version_selector) {
// Set the default flyout menu code
// But only if the user hasn't overridden it already
window.readthedocs.integration_version_selector = function (project) {
// Existing code we use to create the flyout menu
// Currently, this is in `core/static-src/core/js/doc-embed/footer.js:injectFooter`
// The `project` variable is the result of the v3 project API for the current project
};
}

When Read the Docs goes to create the flyout menu,
it will call this new ``readthedocs.integration_version_selector`` method
which will either be our default flyout menu method or the overridden method from the theme or project.

This gives us a baseline of how to add these kinds of integrations.
There are other possible integrations that users may want in the future such as:

* Control how RTD's search overrides Sphinx's built-in search.
This could be used to remove RTD's search overrides entirely
or to just change how they take effect.
* Control how RTD's built-in project analytics sends data.
* There could be an override to how the project's data is retrieved from the API.
This could allow not even getting project/version/translation data at all to save an API call for the project.


Disabling the flyout entirely
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

One nice aspect of this integration is that for projects or themes that want to completely disable
the version selector, this could be done by having JS like this:

.. code-block:: javascript

window.readthedocs = window.readthedocs || {};
window.readthedocs.integration_version_selector = function () {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this custom_version_selector maybe? Wasn't obvious for me that this was a custom function to inject the footer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also call it something other than version_selector since it does a lot more than that.


An alternative would be a way for JS projects to define constants that affects how RTD works.
This could be something like:

.. code-block:: javascript

window.readthedocs = window.readthedocs || {};
window.readthedocs.customizations = {disable_custom_search: true, disable_version_selector: true};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already do this, but we haven't document them. I like more the name you are proposing here p:

READTHEDOCS_DATA.features.docsearch_disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we do this. I like the idea a lot but I wish it was easier for users to control as well as documented. The way it is now can run into race conditions with us defining these features and users setting them.



.. figure:: ../../_static/images/design-docs/flyout/flask-versions-mockup.png
:align: right
:figwidth: 300px
:target: ../../_static/images/design-docs/flyout/flask-versions-mockup.png

Flask documentation with a mockup of a custom version selector on the left sidebar


Implementation steps
--------------------

These are the steps that need to be taken to replace our existing footer API v2.
As much as possible, these steps have been setup so they can be done and rolled out independently
so they don't need to be completed all at once.

* Make the changes to APIv3 to allow requesting translations, sub- and super-projects, and downloads.
* Create a feature flag that will make projects use the new APIv3 instead of APIv2.
Set that feature flag on our own projects.
* Modify our embedded document JavaScript to use a new ``readthedocs`` global variable.
If this new feature flag is set, instead of calling the APIv2, the APIv3 will be called
and then ``readthedocs.integration_version_selector`` will be called with the results.
* If all goes well, remove the feature flag and make APIv3 the default and deprecate APIv2.


Future changes not in this rollout
----------------------------------

* Removing the old ``READTHEDOCS_DATA`` variable is not part of this implementation.
This global will continue to be available but removing it will cause some projects to break for sure.
* This proposal doesn't involve creating an integration point to control custom search.
That could happen at a later date.
* This proposal doesn't rework how the version selector looks either on the RTD Sphinx theme
or on other themes by default. Any restyling can be done independently of this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for this proposal to work for commercial users that are currently manipulating the flyout via CSS rules, we have to output the exact same DOM structure that we would normally return with the HTML blob. This is a little unfortunate, as the DOM structure could already stand to be designed better. We can adapt this later, but this might also hint at some support for multiple versions of DOM output.