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

API: return the api_version on the response #10276

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Apr 28, 2023

This new header will force the API endpoint to return a specific JSON version schema without taking into consideration the any other header.

The idea behind this is to allow theme authors guarrantees about the JSON scheme they are expecting; without stopping us to move forward with potential changes required to the API response when adding new features/addons/etc.

Related readthedocs/addons#64

This new header will force the API endpoint to return a specific JSON version
schema without taking into consideration the any other header.

The idea behind this is to allow theme authors guarrantees about the JSON scheme
they are expecting; without stopping us to move forward with potential changes
required to the API response when adding new features/addons/etc.

Related readthedocs/addons#64
@humitos humitos requested a review from a team as a code owner April 28, 2023 17:47
@humitos humitos requested a review from stsewd April 28, 2023 17:47
@humitos humitos requested review from agjohnson and removed request for stsewd April 28, 2023 17:47
@humitos
Copy link
Member Author

humitos commented Apr 28, 2023

@agjohnson I implemented the new header to support the work done in readthedocs/addons#64. While I was working on this, I thought that maybe we don't want to do two requests and instead we only want to do one and return both JSON structures in the same response.

So, when our JS client asks for the v1 of the JSON and the theme author asks for the v0 of it, I'm thinking we could return something like the following and put event.detail = data[0] so the theme author gets the exact version they asked for:

{
  "0": { },  // ... structure for the v0 of the JSON ...
  "1": { },  // ... structure for the v1 of the JSON ... 
}

The potential downside that I see here is dealing with invalid API versions specified by the theme authors and also error handling. I think there are some trade offs here, but I wanted to mention it because maybe we can discuss it a little more and decide if it's a good idea or not. In any case, I think it's an implementation detail and we can improve its performance later since the JS code is encapsulated.

@agjohnson
Copy link
Contributor

Hrm, I guess it's not clear what the case would be for our JS asking for a different version than what the theme author is asking for. Shouldn't these always be the same version?

That is, shouldn't our addon library inspect what version is defined in a meta tag before requesting the API data?

@humitos
Copy link
Member Author

humitos commented Apr 28, 2023

I guess it's not clear what the case would be for our JS asking for a different version than what the theme author is asking for. Shouldn't these always be the same version?

The idea behind this work is to allow us to keep adding addons, update them, make breaking changes to the API response, etc -- but keeping theme author integrations working properly.

This is, our latest js client may require v3 of the API response but the theme integration may still be using v1. Basically, our js client is independent from the theme integration.

@agjohnson
Copy link
Contributor

I agree on versioning the API response, but I was instead asking why our library would ever request a v3 API response when the theme defined a requirement for a v1 API response. Shouldn't our library only request the v1 API response?

It seems like this is trying to cover the use case where a theme/doc author is both using our API directly and using our built in elements. Is that correct?

I guess I have questions:

  • Do we expect many users to hit this scenario? That is, the user is using our API response directly, needs a specific version of our API, and wants to use the latest versions of all of the rest of our library?
  • Shouldn't theme maintainers be pinned to a specific version of our library (and thus a specific version of our API response)?
  • Does it make more sense for our library to disable functionality if the API response version is defined by the user?

@humitos
Copy link
Member Author

humitos commented Apr 28, 2023

Really good questions. Let's see if I can clarify what's in my head and why I went this way.

I started this work because of the conversation we had with Eric at PyCon, so you are missing some context here. Again, I'm not saying this implementation/pattern is the correct; but it's what I understood from that talk.

The main goal here is to provide theme authors a way to use the JSON schema in a stable way (this is the readthedocsdataready event we talked in readthedocs/addons#64). Theme authors will integrate "some functionality" on their own theme and use all the other addons from Read the Docs. Note that we will always inject our flyout/toolbar/dashboard (whatever it's gonna be called) to have "one and only one Read the Docs experience" in all the sites hosted on our platform. However, as I was saying, theme authors will use the API data to build their own integration: for example, a version selector on the navbar at the top.

I think this scenario will be the second most used case where users use a theme that uses our API response in a custom way to make the theme a little nicer. However, our flyout/toolbar/dashboard is always injected with all our latest addons.

Answering the specific questions:

Do we expect many users to hit this scenario? That is, the user is using our API response directly, needs a specific version of our API, and wants to use the latest versions of all of the rest of our library?

Yes. I think this is going to be the second most used case. I think about integrating the version selector into the theme, but do not overriding the PR and non latest version notification at all.

Shouldn't theme maintainers be pinned to a specific version of our library (and thus a specific version of our API response)?

I don't think so. I want our js client to always be the latest one with all the newest features. This allow us to broadly deploy bugfixes, new addons and more to all our users without breaking theme integrations that may rely on an older API data response.

Let's say that we create a new addon that shows a new notification and requires v2 of the API data response. We deploy the newest js client. It will request v2 for its own usage and also v1 for the theme integrations to keep working without breaking.

Does it make more sense for our library to disable functionality if the API response version is defined by the user?

I don't think so. We always want to show our own flyout/dashboard/toolbar (see readthedocs/addons#53) with all our addons. This is what we called at PyCon "the Read the Docs unified experience"


Does this help to clarify the vision/idea/path forward that we are proposing here?

@humitos
Copy link
Member Author

humitos commented Sep 19, 2023

After doing a POC 🧪 that uses the readthedocs-addons-data-ready event in our own Sphinx theme to integrate the flyout into the navigation bar, copying the current look&feel (see readthedocs/sphinx_rtd_theme#1526), I'd like to move forward with this implementation/pattern. After working on the shoes of a "theme maintainer", I can say it's pretty clear to me and it has a lot of benefits for everybody. Mainly, we are not "tied" to a specific JSON structure that we have to maintain forever without being able to touch it at all. This allows us to move forward and allow them to be sure their integration will keep working in the future. Let me explain some details:

As a theme author, you should:

  • define the expected API response version via <meta ... content="1.0"> (mandatory)
  • subscribe to readthedocs-addons-data-ready
  • use the data coming in event.detail as you want (we guarantee you that JSON structure will be 1.0)

As Read the Docs developers, this allow us:

  • commit ourselves to always give theme authors the exact same JSON structure (it's versioned and explicitly defined)
  • introduce breaking changes in the JSON structure, create a new version (e.g. 2.0) and use it in new addons
  • themes using an older version (e.g. 1.0) will get the JSON structure they were expecting

As a small example, let's say the theme author integrates the flyout sections separately: the language selector is at the top right and the version selector is at the top left in the navbar (just to mention something) by using the APIv1 JSON structure. All the other addons are kept with their default values and no extra integrations/modifications are done.

After some time, we found an issue in the flyout addons that require some breaking changes on its JSON structure and we want to fix it. We make them, create APIv2 JSON structure and update our addons.js file to request for the APIv2 JSON structure and use it.

At this point, when the theme that integrates the flyout is loaded, our addons.js will make use of APIv2 JSON structure for our own addons and will keep exposing APIv1 JSON structure under the readthedocs-addons-data-ready so the theme can continue working with the exact JSON structure it expects 🚀

In the future, if the theme author wants, they can update its meta tag to be <meta ... content="2.0"> and update its code to make usage of the new APIv2 JSON structure ✨

@humitos humitos changed the title API: accept X-RTD-Hosting-Integrations-API-Version API: return the api_version on the response Sep 20, 2023
@humitos
Copy link
Member Author

humitos commented Sep 20, 2023

I changed the title because the api-version= explicit parameter was implemented in #10753

The only thing that was not implemented there was returning the api_version field in the JSON response. So, this PR only does that.

@agjohnson
Copy link
Contributor

By the way, this plan makes a lot of sense so far. I think you are correct in serving a static version of the addons JS, but with a variable data structure. We'll have to see what weird things theme authors start doing with our addons eventually, but I suspect it won't be a problem for theme authors to rely on just the data structure.

I'm thinking that if we do need to allow theme authors access to the addons API (not just the data structure) they should pin our library and bundle the functions they need into their own JS. We'd still inject and use the latest addons JS though. That's something we can cross later, I don't see a strong need for this yet.

@humitos humitos enabled auto-merge (squash) September 21, 2023 10:10
@humitos humitos merged commit a847397 into main Sep 21, 2023
4 checks passed
@humitos humitos deleted the humitos/api-hosting-version branch September 21, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants