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.

Sign up for GitHub

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: use APIv3 endpoint for resources #468

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

API: use APIv3 endpoint for resources #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a18ca99
API: use APIv3 endpoint for resources
humitos Dec 9, 2024
ddeb084
Mix addons API endpoint with APIv3 endpoints
humitos Dec 12, 2024
d19d740
Hit the endpoint for filetreediff
humitos Dec 12, 2024
b539256
Use APIv3 URLs coming from the `/_/addons/` endpoint
humitos Dec 16, 2024
e6e8309
Merge branch 'main' into humitos/apiv3-endpoints
humitos Jan 9, 2025
737d6f7
Explain why we are getting only 10 results
humitos Jan 9, 2025
cd037de
We are using the `?limit=` paramenter in the endpoint now
humitos Jan 9, 2025
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
8 changes: 6 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { getReadTheDocsConfig } from "./readthedocs-config";
import {
getReadTheDocsConfig,
getReadTheDocsConfigUsingAPIv3,
} from "./readthedocs-config";
import * as notification from "./notification";
import * as analytics from "./analytics";
import * as search from "./search";
Expand Down Expand Up @@ -50,7 +53,8 @@ export function setup() {
}
}

return getReadTheDocsConfig(sendUrlParam);
// return getReadTheDocsConfig(sendUrlParam);
return getReadTheDocsConfigUsingAPIv3(sendUrlParam);
})
.then((config) => {
const loadWhenEmbedded = objectPath.get(
Expand Down
60 changes: 60 additions & 0 deletions src/readthedocs-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,66 @@ export function getReadTheDocsConfig(sendUrlParam) {
});
}

export async function getReadTheDocsConfigUsingAPIv3(sendUrlParam) {
const defaultApiUrl = _getApiUrl(sendUrlParam, ADDONS_API_VERSION);
const addons = await (await fetch(defaultApiUrl)).json();

const projectResponse = fetch(
addons.readthedocs.urls.api.v3.projects.current,
);
const translationsResponse = fetch(
addons.readthedocs.urls.api.v3.projects.translations,
);

const versionResponse = fetch(
addons.readthedocs.urls.api.v3.versions.current,
);
const activeVersionsResponse = fetch(
addons.readthedocs.urls.api.v3.versions.active,
);
const buildResponse = fetch(addons.readthedocs.urls.api.v3.builds.current);
const filetreediffResponse = fetch(
addons.readthedocs.urls.api.v3.filetreediff,
);
Comment on lines +150 to +152
Copy link
Member

Choose a reason for hiding this comment

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

I worry a little bit about one of the API calls failing and leading to weird state. Would be good to put a TODO in here probably to do a retry?

Copy link
Member Author

@humitos humitos Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I need to re-write this code to handle errors as well. Currently, if an API call break, the whole code breaks completely 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

@agjohnson what's the best pattern to use for this chunk of code? async/await Promises (as it's written) or then-ables?

My goal here is to make multiple requests in parallel to APIv3 while handling errors. Besides, once that all requests have responded, I want to use the respond data to form the config object with the same structure we currently have.

Copy link
Contributor

Choose a reason for hiding this comment

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

What you have is close, though yeah it does need more error handling -- especially using fetch() you should check for resp.okay.

One thing though is that this is sort of written more like synchronous code. Normally, you'd want to take advantage of the evented code flow more, and instead of doing operations in bulk and await on each of these blocks, you'd let the request and encoding and any other operation run in a single promise.

That is, this is executing:

fetch(), fetch(), fetch(), fetch()
resp.json(), resp.json(), resp.json(), resp.json()

Where a fully evented flow is more like:

fetch(), resp.json()
fetch(), resp.json()
fetch(), resp.json()
fetch(), resp.json()

The effect can be subtle, but it does allow for early failure if one request or request response is bad.

What you have here overall is good so far, though I think through restructuring to add some error handling and simplifying things, you'll probably end up with a separate async function for a single request and request encoding.


const responses = await Promise.all([
projectResponse,
translationsResponse,
versionResponse,
activeVersionsResponse,
buildResponse,
filetreediffResponse,
]);

const [project, translations, version, activeVersions, build, filetreediff] =
await Promise.all(responses.map((response) => response.json()));

Object.assign(addons, {
builds: {
current: build,
},
projects: {
current: project,
translations: translations.results,
},
versions: {
active: activeVersions.results,
current: version,
},
});

Object.assign(addons["addons"]["filetreediff"], filetreediff);

// Trigger the addons data ready CustomEvent to with the data the user is expecting.
dispatchEvent(
EVENT_READTHEDOCS_ADDONS_DATA_READY,
document,
new ReadTheDocsEventData(addons),
);

return addons;
}

function dispatchEvent(eventName, element, data) {
const event = new CustomEvent(eventName, { detail: data });
element.dispatchEvent(event);
Expand Down