Skip to content

Build: support build time injected logic via generic HTML injection #9063

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

Closed
5 of 6 tasks
agjohnson opened this issue Mar 31, 2022 · 15 comments
Closed
5 of 6 tasks

Build: support build time injected logic via generic HTML injection #9063

agjohnson opened this issue Mar 31, 2022 · 15 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Mar 31, 2022

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

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

Refs #9062

@agjohnson agjohnson added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Mar 31, 2022
@agjohnson agjohnson moved this to Planned in 📍Roadmap Mar 31, 2022
@agjohnson
Copy link
Contributor Author

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.

@ericholscher
Copy link
Member

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

@ericholscher
Copy link
Member

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

@humitos
Copy link
Member

humitos commented Jul 8, 2022

@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

@agjohnson
Copy link
Contributor Author

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.

@ericholscher
Copy link
Member

ericholscher commented Nov 23, 2022

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

@humitos
Copy link
Member

humitos commented Nov 23, 2022

@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?

@agjohnson
Copy link
Contributor Author

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.

@ericholscher
Copy link
Member

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.

@agjohnson
Copy link
Contributor Author

agjohnson commented Nov 23, 2022

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.

@ericholscher
Copy link
Member

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.

@agjohnson
Copy link
Contributor Author

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.

@humitos
Copy link
Member

humitos commented Nov 24, 2022

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.

@ericholscher
Copy link
Member

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)

@humitos
Copy link
Member

humitos commented Mar 11, 2023

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 humitos closed this as completed Mar 11, 2023
@github-project-automation github-project-automation bot moved this from Planned to Done in 📍Roadmap Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

No branches or pull requests

3 participants