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

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

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 9, 2024

Initial implementation as a POC to give it a try. It works fine at first sight and I think it could be a good idea to move forward with it. I didn't find any blocker yet.

Closes #356
Requires readthedocs/readthedocs.org#11831

Initial implementation as a POC to give it a try. It works fine at first sight
and I think it could be a good idea to move forward with it. I didn't find any
blocker yet.

Closes #356
Comment on lines +150 to +152
const filetreediffResponse = fetch(
addons.readthedocs.urls.api.v3.filetreediff,
);
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

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.

src/readthedocs-config.js Outdated Show resolved Hide resolved
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.

API: consider using APIv3 standard endpoints
3 participants