readthedocs.org icon indicating copy to clipboard operation
readthedocs.org copied to clipboard

Build: support build time injected logic via generic HTML injection

Open agjohnson opened this issue 2 years ago • 4 comments

In #8190 we described generalizing our build process with generalized HTML manipulation instead of Sphinx-specific solutions for injection.

I can't summarize here, so might leave this to @humitos to expand more. TBD

  • [ ] Flyout, CSS, and JS injection in HTML files (most of these are done in readthedocs-sphinx-ext at this file https://github.com/rtfd/readthedocs-sphinx-ext/blob/7cc1e60f7dcdeb7af35e3479509a621d5bac0976/readthedocs_ext/readthedocs.py)
  • [x] Search indexing of HTML
  • [ ] Version warnings (https://github.com/rtfd/readthedocs-sphinx-ext/blob/7cc1e60f7dcdeb7af35e3479509a621d5bac0976/readthedocs_ext/versionwarning.py)
  • [ ] External version warnings (https://github.com/rtfd/readthedocs-sphinx-ext/blob/7cc1e60f7dcdeb7af35e3479509a621d5bac0976/readthedocs_ext/external_version_warning.py)
  • [ ] ???

This sounds complex enough that we should have two issues to track probably.

Refs #9062

agjohnson avatar Mar 31 '22 00:03 agjohnson

A few notes regarding this issue from a meeting today:

  • We're implementing build.commands without a user contract file initially. The implementation will use convention for now
  • We might still need a contract file for HTML upload to work
  • We'll be discussing this more after an initial implementation of build.commands is usable, and we have an idea of what limitations users are hitting with a convention based approach.

agjohnson avatar Apr 20 '22 18:04 agjohnson

We have already shipped the search parsing for build.commands.

ericholscher avatar Jul 07 '22 18:07 ericholscher

The next step here is documenting how to implement these features with build.commands in some fashion. Perhaps just by installing our extension manually?

ericholscher avatar Jul 07 '22 18:07 ericholscher

@ericholscher yes, I'd say that's the first step. We can later improve the integration if that makes sense, but communicating to people how to get the same result they get with the regular builder is a good step that gives users a solution without involving us in writing a lot of that integration yet.

I think we have two options here:

  1. expose the readthedocs-sphinx-ext to users
    • this seems to be the simplest way
    • it only works with projects building with Sphinx
    • it will force us to maintain the extension to be used in multiple different ways that may not be prepared
  2. document the "behind the scene" and let users handle/implement them
    • document what exactly each of the extension does (eg. add an HTML block at the top with this content, includes a particular JS and CSS for the flyuout, another for the Ads, etc)
    • this will help us to create the final input/output contract we want for builders
    • let users integrate this as they prefer, using the tools they feel comfortable with
    • work will multiple doctools

humitos avatar Jul 08 '22 11:07 humitos

Note here: we probably should have a design discussion on how to accomplish injection of these pieces. We may be able to do this at proxito, injection at build time is a possibility, and there is also fancier versions of this with web workers, but that is likely too technical of an implementation.

agjohnson avatar Nov 23 '22 01:11 agjohnson

We also have https://github.com/readthedocs/meta/issues/71 for doing the documentation as a first step.

ericholscher avatar Nov 23 '22 01:11 ericholscher

@agjohnson @ericholscher the more I think about this, the closer I get to the idea of "creating a Read the Docs javascript library that handles all of them". Similar to what we do we EthicalAds, but for the the warning banners and other hosting integrations.

I think that injecting a blob of HTML at build time, include a js/css file as well, and then (maybe) add extra js/css at serving time it's the wrong approach. I'd like to have all of this standardized and it seems that javascript seems to be the right place to do this making it doctool agnostic. What do you think?

humitos avatar Nov 23 '22 15:11 humitos

Yeah, I probably lean that way, assuming we have a slightly more established JS library pattern and break out all of this to a standalone library. I still don't know if I want this at build time or injection time though. That could be a good discussion to have.

  • https://github.com/readthedocs/meta/issues/96
  • https://github.com/readthedocs/readthedocs.org/pull/8052

agjohnson avatar Nov 23 '22 18:11 agjohnson

Yea, that is already what we do with the readthedocs-doc-embed.js file. That integration should be enough for everything we need to get a site RTDified.

I'm definitely on the side of injecting it at serving time, since that gives us a lot more options going forward. It would work similarly to the flyout now with the user being able to tell us where to inject it, or we inject it at the bottom of the <head>. I think CloudFlare functions is the right level to do this, I'm guessing, but not 100% sure.

ericholscher avatar Nov 23 '22 19:11 ericholscher

I'm more torn, because there is a tradeoff to injecting at build time so that the built artifact never changes in the future (we don't have to support old documentation implementations with new client JS releases) and injecting at build time so that our implementation is guaranteed to be the same (but we do have to worry about backwards compatibility).

There is a middle ground solution, and it's probably what I'm describing when I say I want the embed client JS to be versioned, and so every build refers to a specific embed client API version or release. We could even backport changes at that point. In this case I lean towards injection at display time.

This is a good deal of package maintenance work however. So, this is a conversation we need to discuss a bit more, in my opinion.

agjohnson avatar Nov 23 '22 19:11 agjohnson

Yea, the current solution we have is injection at build time, but to an unversioned file that changes over time. Versioning that file is definitely an interesting use case, but I do think we need the ability to roll things out across all hosted docs at once in some fashion.

ericholscher avatar Nov 23 '22 19:11 ericholscher

Aye, that middle ground solution I'm describing would be maintaining multiple versions of the client library package, and also hosting multiple major versions in a shared place. It's a bit more maintenace work, but I doubt we'd have too many cases where we have to backport changes. The multiple hosted versions would mostly be for snapshotting the major versions so we can stop worrying about historical support.

That is:

  • assets.readthedocs.org/../embed/1.0/client.js and client.css
  • assets.readthedocs.org/../embed/2.0/client.js and client.css
  • etc

I suppose at that point, build time injection could add these includes in <head> at build, display time injection would add them via Proxito most likely. But the two solutions could be mostly comparable. We'd have the option of deploying a 1.1 bugfix to ../embed/1.0/client.js and have it live on all old sites/etc.

agjohnson avatar Nov 23 '22 20:11 agjohnson

If we version the library and we have metadata about the project like doctool, doctoolversion and theme (data coming from the build contract) we could keep different versions compatible over time.

That would allow us to inject an old version of our library, for projects building today with Sphinx 2.x for example. Also, in the case we inject these files dynamically at CF, we can make the same decision based on this data.

humitos avatar Nov 24 '22 10:11 humitos

Notes from our call on this here: https://docs.google.com/document/d/1Cs5uW7TD5jaUkrFsVMz5YtKF0iAlnpktxxc-FhSHCQc/edit#heading=h.srhzjalnqm6z (RTD Internal only, but I can copy/paste here if folks are curious)

ericholscher avatar Nov 30 '22 18:11 ericholscher

The work described originally in the description is happening in #10127. Besides, in this issue we start taking about where to inject the JS, which is also happening in #10127.

Finally, we detoured to talk about versioning the flyout JavaScript, which that conversation is happening in https://github.com/readthedocs/meta/issues/96

So, I'm closing this issue aiming for simplification and easy to follow conversations/discussions for each topic in just one place while keeping all the issues/PR/discussion linked between each of them in case we want come back to review the context.

humitos avatar Mar 11 '23 12:03 humitos