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

Docs: embed API #8107

Closed
wants to merge 4 commits into from
Closed

Docs: embed API #8107

wants to merge 4 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Apr 14, 2021

Wasn't sure where to put the "Others APIs", think we all agreed these APIs are different from the ones we have in rtd.org/api/

Usage
-----

Check our :doc:`/guides/embedding-content` guide to learn how to use the embed API in your projects.
Copy link
Member Author

Choose a reason for hiding this comment

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

I was tempted to move the content from this guide to this section...

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to have this split. This page is the "raw" documentation of the API's endpoint and the other is a guide that shows some examples.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is a good start!

Usage
-----

Check our :doc:`/guides/embedding-content` guide to learn how to use the embed API in your projects.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to have this split. This page is the "raw" documentation of the API's endpoint and the other is a guide that shows some examples.

Comment on lines 14 to 18
Additionally, you can use the API with:

- `sphinx-hoverxref`_: Sphinx extension

.. _sphinx-hoverxref: https://sphinx-hoverxref.readthedocs.io
Copy link
Member

Choose a reason for hiding this comment

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

This probably can be removed from here since it's already mentioned in the guide linked above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think is still useful to link to users directly to the extension, so they don't need to click around to use the extension

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we are duplicating content here that we need to maintain. If any, I think we could improve the guide to make the extension more prominent.

Copy link
Member Author

Choose a reason for hiding this comment

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

we are duplicating content here that we need to maintain.

But this is just a link :) and the docs about the extension and how to use it should be on the extension itself, not on rtd.

Comment on lines 23 to 24
The embed API is exposed from the domain where your docs are being served.
This is ``https://docs.readthedocs.io/_/api/v2/embed`` for the ``docs`` project, for example.
Copy link
Member

Choose a reason for hiding this comment

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

The endpoint also works at https://readthedocs.org/api/v2/embed/. I'm not sure if we want all people using the endpoint from the documentation's domain. Why is that required?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you can avoid CORS issues and use the cookies from the same domain in private projects.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think CORS should be open and less restrictive --which I don't think will be a problem. However, cookies are required for private documentation on commercial, tho.

If you are using :ref:`private versions <versions:privacy levels>`,
users will only be allowed to get the embed content from projects they have permissions over.
Authentication and authorization is done using the current session,
or any of the valid :doc:`sharing methods </commercial/sharing>`.
Copy link
Member

Choose a reason for hiding this comment

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

It may be good to be explicit that the sharing methods are only useful for commercial.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is note about that at the top of that page

docs/embed-api.rst Outdated Show resolved Hide resolved
docs/embed-api.rst Outdated Show resolved Hide resolved
docs/embed-api.rst Outdated Show resolved Hide resolved
docs/embed-api.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member

humitos commented Jun 1, 2021

I was thinking if it does make sense to publish the documentation for the Embed APIv2 now that we are deprecating it and working on a new APIv3? I'm not sure if we want people to discover it at this point and start building new tools with it.

@stsewd
Copy link
Member Author

stsewd commented Jun 1, 2021

I was thinking if it does make sense to publish the documentation for the Embed APIv2 now that we are deprecating it and working on a new APIv3? I'm not sure if we want people to discover it at this point and start building new tools with it.

I think people are already using the embed api, but not sure if they are using it without the client, or at least @ericholscher has a .com client using it.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems like useful work. I think we're gonna need to keep supporting v2 in some fashion for a long time, given the amount of users already using it via hoverxref. Since it's already documented, having it be better docs feels useful to me. Hopefully this structure will also give us a bit more ability to deprecate it over time.

I don't feel strongly here though, but dislike throwing away this work if it's useful. @humitos will let you make the decision tho, as you're on the redesign.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This seems like useful work.

The work is useful. However, I think we changed directions in the following 2 months after the PR was created and now, with the current direction decided, I think we shouldn't promote API v2 anymore. The less we promote it the easier will be to deprecate it.

I think we're gonna need to keep supporting v2 in some fashion for a long time, given the amount of users already using it via hoverxref.

This is true, but we are the ones that have control over that extension. Once APIv3 is implemented we can immediately make the change and release a new version of hoverxref --users will gradually upgrade to the new version and we will be closer to remove v2.

On the other hand, having new users implementing new tools using the v2 all around the internet will drastically increase the deprecation problems in my opinion. In that scenario, we need to migrate the users of our extension and also all the users of these new tools that we don't know who they are and we don't have a good way to contact them either.

I don't feel strongly here though, but dislike throwing away this work if it's useful. @humitos will let you make the decision tho, as you're on the redesign.

I'm sorry, but I vote to not publish this content under our public documentation. We should share a PDF with this document to any user already wanting to build something with it that we can contact later, and warn them about the current status so they can make the decision by themselves.

I hope this makes sense to you.

(I left some other comments related to this in the review itself).

Comment on lines +10 to +16
Usage
-----

Check our :doc:`/guides/embedding-content` guide to learn how to use the embed API in your projects.
You can also use the API with our `sphinx-hoverxref`_ Sphinx extension.

.. _sphinx-hoverxref: https://sphinx-hoverxref.readthedocs.io
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to remove this section because this document is about the raw API (similar to the regular APIv3 where we are not showing use cases). Let's keep consistency here.

If you want to cross-reference to our guide, you can use a note/tip with the first sentence linking to it. Besides, I wouldn't mention sphinx-hoverxref here since it's one of the topics the guide already talks about: we are duplicating content.


.. warning::

This API isn't stable yet, feel free to `open an issue`_ if you find any problems.
Copy link
Member

Choose a reason for hiding this comment

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

This warning shows my point here. We are documenting, and promoting its usage, for an API and on the same page we are telling users "don't use it because it's not stable". We are not planning to make it stable either. We are planning to deprecate and replace it with APIv3. So, I don't think why it would be useful to document it for more users to discover it?

If there is a current user wanting to use it, we should share this document privately with them, warning them it will be replaced in the medium term and they should keep that in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It seems like we will want this post once embed endpoints are available on apiv3.

docs/api/embed.rst Show resolved Hide resolved
Comment on lines +25 to +26
There are some undocumented endpoints in our APIs that are for internal usage.
These should not be used and could change at any time.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph reflects the current state of the Embed API better than the documentation proposed in this PR to me 😄

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Agreed this seems like something we want once the embed endpoints are on apiv3. Seems like we can come back to this PR and update it however, instead of throwing away the work here?


.. warning::

This API isn't stable yet, feel free to `open an issue`_ if you find any problems.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It seems like we will want this post once embed endpoints are available on apiv3.

@@ -0,0 +1,135 @@
Embed API
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're waiting until embed endpoints are on APIv3, this should go with our other APIv3 endpoint docs at docs/api/v3.rst

docs/api/embed.rst Show resolved Hide resolved
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If you are using :ref:`private versions <versions:privacy levels>`,
users will only be allowed to get the embed content from projects they have permissions over.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it's maybe changing with CORS changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this doesn't makes reference to external site, but to requests from the same site.


If you are using :ref:`private versions <versions:privacy levels>`,
users will only be allowed to get the embed content from projects they have permissions over.
Authentication and authorization is done using the current session,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Authentication and authorization is done using the current session,
Authentication and authorization are done using the current session,

@humitos
Copy link
Member

humitos commented Jun 14, 2021

OK. I'm closing this PR for now. We may come back to it once APIv3 is implemented to re-open and update it or take some content for a new document.

@humitos humitos closed this Jun 14, 2021
@stsewd stsewd deleted the docs-embed branch June 14, 2021 14:21
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.

4 participants