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

Improvements for the embed app #7963

Closed
stsewd opened this issue Feb 25, 2021 · 5 comments · Fixed by #8222
Closed

Improvements for the embed app #7963

stsewd opened this issue Feb 25, 2021 · 5 comments · Fixed by #8222
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required

Comments

@stsewd
Copy link
Member

stsewd commented Feb 25, 2021

Wanted to put this somewhere:

  • Use the proxied endpoint and set cache tags, for faster loads
  • Return 404 for unknown sections
  • only support URL and fragment, not doc, page, section, etc, that is a sphinx specific concept
  • support mkdocs
  • Don't return all headers, maybe have another endpoint to return all headers
  • Store sections in the db/storage, so lookups are faster (right now we are at ~500 ms)
  • Return a single string as the content rather than a list of strings (looks like this is always a list of a single element anyway...)
  • Return the whole id of the elements (for example, for the first id we return "#" instead of the full id)
  • Don't include "#" in the id response
  • Include the section id and section title in the response
  • Just do one look up (right now we are doing 5: the id itself, three variants of the id, and the id used as the title)
  • Always return valid html (we are returning dd tags without a dt tag nor inside a dl tag)
  • Return a list of extra js/css files, I've seen we are doing this in the extension side, but if we want to inject this outside docs pages, we may want to also return what js/css files are required (this may open some security issues, but they are to the client to decide if they should be injected or not). An example of this is sphinx-tabs, we need to inject some extra js in order to maked them work outside the docs pages.
  • Don't return nested sections, only the section itself (or maybe a limit of subsections?)

Some of those are breaking changes, others are less breaking the others. Right now our extension is the only user of this undocumented api so far I think. Since it's undocumented I think we can break it a little (at least the no so breaking changes)

@stsewd stsewd added Improvement Minor improvement to code Needed: design decision A core team decision is required labels Feb 25, 2021
@ericholscher
Copy link
Member

@stsewd > Use the proxied endpoint and set cache tags, for faster loads

Strong 👍 on cache tags & CDN. That's a huge selling point of this to me.

Return a single string as the content rather than a list of strings (looks like this is always a list of a single element anyway...)

I think we should figure out the exact API response we want and migrate to it ASAP. As we start building more integrations on it, we're gonna have to maintain the existing API longer :/

@stsewd
Copy link
Member Author

stsewd commented Mar 8, 2021

I think we should figure out the exact API response we want and migrate to it ASAP. As we start building more integrations on it, we're gonna have to maintain the existing API longer :/

Yeah, if we are fine breaking the API now, I can put together a proposal on how the response and params look like.

Also, updated the issue to also talk about extra js/css files :D

@humitos
Copy link
Member

humitos commented Mar 9, 2021

Yeah, if we are fine breaking the API now, I can put together a proposal on how the response and params look like.

No. We shouldn't break the API --sphinx-hoverxref depends on it and so many projects. New API response should be under /v3 if it's going to be different.

@humitos
Copy link
Member

humitos commented Mar 29, 2021

Store sections in the db/storage, so lookups are faster (right now we are at ~500 ms)

I don't think storing sections on the db scales. Using the storage for storing sections will have around the same timing that fetching the whole HTML and parse it. I'd say it doesn't worth the effort for this.

Don't return nested sections, only the section itself (or maybe a limit of subsections?)

I want to return nested sections. We should accept more arguments in the endpoint so we can limit the amount of content returned.

@ericholscher
Copy link
Member

I don't think storing sections on the db scales. Using the storage for storing sections will have around the same timing that fetching the whole HTML and parse it. I'd say it doesn't worth the effort for this.

Yea, I'm -1 on storing this data in the DB for sure. We should use CDN caching to make it faster, if anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants