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

Update API.md with correction and new info #676

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mikemorran
Copy link

Corrections:

  1. Note about alpha testing removing note that users cannot create their own tokens.

New Info:

  1. Line about where to generate tokens.
  2. Write rooms with info about customizable room features taken from graphQL schema in room_types.ex

Corrections: 
1. Note about alpha testing removing note that users cannot create their own tokens.

New Info:
1. Line about where to generate tokens.
2. Write rooms with info about customizable room features taken from graphQL schema in room_types.ex
@mikemorran
Copy link
Author

I see that I have a few markdown errors...

guides/api.md Outdated
@@ -1,7 +1,7 @@
# Hubs Server API
Reticulum includes a [GraphQL](https://graphql.org/) API that grants programmatic access to server resources.

Note: This API is currently in alpha testing and is not yet available for use. (Users cannot generate API Access Tokens.)
Note: This API is currently in alpha testing and will likely experience frequent updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I might just say, "This API is not yet stable, and may undergo breaking changes in future updates."

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

guides/api.md Outdated
- `user_data` _**map**_ \| Arbitrary json data associated with this room.

See [room_types.ex](../lib/ret_web/schema/room_types.ex) for full GraphQL Schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for writing this.

I am torn on whether to include it, because GraphQL has built-in __schema field that allows users to learn about a query: https://graphql.org/learn/introspection/

The built-in method has no risk of being outdated or wrong as we change things, whereas having this data duplicated here means that we risk forgetting to update it, or making a mistake.

I would not want to recreate the documentation that the graphql team already worked hard to write -- Perhaps we should link the graphql documentation and include some introspection queries in our sample workspace file.

Copy link
Author

Choose a reason for hiding this comment

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

I did not know about this and agree we should utilize this tool which will prevent us from having to constantly update. I will review the docs tomorrow and put together a bit more of a step-by-step of how to quickly see what is available.

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

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

Leaving an "approve" review despite my comments because I do not want you to be blocked from proceeding if this is what you want to deploy.

The "reviews require explicit approval" rule is only in place to protect ourselves from running database migrations that we would regret. This PR does not have that risk whatsoever.

I decided to remove the list of writable parameters in favor of John's suggestion to include the introspection tools. I have also linked a new version of the workspace which is simplified to 5 tabs, including introspection queries.

QUESTION @ john & bryan: Why did the older example workspace include tabs for both app and user tokens? This was confusing to me, but I don't want to omit any important materials, especially if there is an important distinction to be made in this example space.
@mikemorran
Copy link
Author

@johnshaughnessy @bryanenders I have updated the doc and added a new sample workspace based on John's recommendation to share the introspection tools.

The sample workspace I added also removes the duplicate tabs for [app] or [user] scopes. I did this because it seemed confusing and maybe a bit redundant, but I feel like I might be missing something important about distinguishing the two.

@bryanenders
Copy link
Contributor

bryanenders commented Feb 17, 2023

IIRC there is a vulnerability in GraphiQL, so it may be going away. I’ll have to dig to confirm. The good news is that the queryType and mutationType should be queryable. Those are the types that make up the schema.

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.

3 participants