search

readthedocs / doc-diff

Read the Docs documentation visual diff
0 stars 0 forks source link

Pattern for injection #2

Open agjohnson opened 2 years ago

agjohnson commented 2 years ago

Originally, we discussed setting this up as a Sphinx extension, but now I'm not sure that's the best option. So far, I've written this to be just dropped in on a page, and the READTHEDOCS_DATA informs the configuration for the most part (with some hacks). With more complete context data, it would be no problem to configure this library entirely from READTHEDOCS_DATA.

At that point, we have a number of ways to inject this script:

I don't have strong preferences here, but automatic injection on a feature flag would allow us to control scale out.

humitos commented 2 years ago

Originally, we discussed setting this up as a Sphinx extension, but now I'm not sure that's the best option

I don't think this has to be a Sphinx extension. I agree with that.

Require users to manually inject this in their own builds to start

At this current stage, I would vote to avoid magic injection and require users to manually inject this. This is a simple and controlled way for users to opt-in for this feature. I think it's enough to document how to include this extension on Sphinx and MkDocs:

  if os.environ.get("READTHEDOCS", "True") and os.environ.get("READTHEDOCS_VERSION_TYPE", "external"):
    app.add_js_file(" ... our js file ...")

Those two lines are pretty simple, I'd say.

agjohnson commented 2 years ago

That certainly does work, I would say this is my backup plan currently. It is an okay alpha or beta stage option, but the upgrade path to a fully integrated feature would be awkward. But in this case, I would make this a Sphinx extension so that we control the logic you mention.

However, given this will strictly be a platform feature, it feels like something that we'll want to automatically inject for projects/doc views eventually. This will also give us control over the version of the library that is used. Starting there doesn't seem bad either, as configuration could just as easily be a feature flag or opt-in project feature too.

I suppose we could write the extension with this upgrade in mind from the first release, and only enable the extension if some variable/value is not yet set. When we start injecting the diff library, we also start injecting the variable/value to builds that will disable the extension. I suppose this all feels like working around injecting the library though.

humitos commented 2 years ago

However, given this will strictly be a platform feature, it feels like something that we'll want to automatically inject for projects/doc views eventually. This will also give us control over the version of the library that is used. Starting there doesn't seem bad either, as configuration could just as easily be a feature flag or opt-in project feature too.

IMO, this will make us to freeze the development of this feature because making an upgrade will break multiple projects. We already have experience by managing dependencies on behalf the user and we have experienced a lot of problems with that approach. I'd prefer to avoid that pattern when possible. I like more the idea of building the tools/extensions, define and document a simple way to integrate them, and leave the user to follow the steps to integrate it as they prefer (using the doctool they want). With that pattern, we can then create a Sphinx extension we control to make the integration easier for users and use SemVer versioning to manage the changes, by making improvements and breaking ones.

In particular with JavaScript extensions, I think it will be pretty hard to know if an upgrade of the extension does break people's projects or not because we won't have a build failing or similar that we can detect immediately. Also, if we follow dynamic injection, old built versions could break when releasing a new version of the JavaScript extension.

The only thing that makes me think a little different for this particular case, is that this extension only affects Pull Requests, which is something that's "moving" and updating all the time but... I still consider this a risky thing to add magic for.

agjohnson commented 2 years ago

I like more the idea of building the tools/extensions, define and document a simple way to integrate them

This is a good pattern for extensions that provide documentation authoring features, as it's altering the result of the documentation project and there is a lot that the user wants control over. As a beta test pattern for this library, it's also okay.

I do agree with you generally here too, regarding maintenance ease, but for platform features I'm a bit more conflicted.

Long term, asking users to opt into platform features results in less users using the feature. It definitely results in disparate experiences with our platform features, as users can use old, broken releases and we have no control over this. Our search extension is a good example of this, and hasn't seen the adoption we wanted for it. The plan to start as an opt-in feature certainly helped mature the extension though.

I do view this feature like the flyout, not like hoverxref. That is, this library provides a platform feature that we control, not a content authoring feature that users control. It's also only usable in pull requests on Read the Docs. While we will have some concerns about compatibility, I view this feature as either on or off for a project (or documentation viewer, really). Users aren't able to opt into a specific version of this (similar to our flyout), and we aren't maintaining the library with disparate experiences in mind. The trade off is that the feature is available for any project, for any documentation reader, immediately.

IMO, this will make us to freeze the development of this feature because making an upgrade will break multiple projects.

I think identifying concerns here, and addressing them proactively, might buy us the most here. Our flyout is a good example of a platform feature, but we missed the opportunity to make it customizable and extendable. This definitely adds to maintenance burden, but maybe most of that is indirectly -- users are now forced to use awful hacks to customize, instead of customizations that could have been backwards compatible from the start.

agjohnson commented 2 years ago

I think identifying concerns here, and addressing them proactively, might buy us the most here

For instance, if we make the normal injection automatic, but give users the ability to pre-inject a custom version of the doc diff library, that seems like a good compromise.

That would involve making this library aware of itself, and if it's been injected/requested already. I don't know exactly what I'd do here, as async loading will make this tricky to do deterministically, but it sounds very possible.

humitos commented 2 years ago

Another thing that I want to mention here regarding automatic injection is that project maintainers (or reviewers) should have a way to opt-out from this extension. Not everybody will like this feature and they should have a way to opt-out. Following the auto-injection patter requires us to think more about this and to build this feature.

agjohnson commented 2 years ago

Well, so I guess the difference here is that I'm describing a documentation reader feature -- like the flyout/search -- and not a project feature.

We don't need to get too complex with a setting for this. A simple project settings with okay UX for the project maintainer might be a setting like:

default project diff state = [full diff by default, additions only by default, diff off by default]

And diff off by default is probably the default setting, as visual diff might be rather overwhelming for a lot of PRs.

But the user would still be able to override this, toggling between the options. That is, the project doesn't outright disable the ability of a reader to toggle this feature on or off. In this case, we can build other features on top of the diff view, like linking to the diff view from our pull requests action.