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

[WIP] add shopify-app-remix scopes API documentation #1506

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

Conversation

RyanDJLee
Copy link
Contributor

@RyanDJLee RyanDJLee commented Sep 13, 2024

Addresses: https://github.com/Shopify/develop-app-runtime-primitives/issues/496

WHY are these changes introduced?

Spin url: https://shopify-dev.shopify-dev-86xc.ryan-d-lee.us.spin.dev/docs/api/shopify-app-remix/v3/apis/scopes
(resume on Spin Control if necessary)

WHAT is this pull request doing?

image
image
image
image

Type of change

Documentation

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used pnpm changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@@ -32,6 +32,7 @@ const data: ReferenceEntityTemplateSchema = {
'EmbeddedAdminContext',
'AdminApiContext',
'BillingContext',
'ScopesApiContext',
Copy link
Contributor

Choose a reason for hiding this comment

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

The page with these examples is so weird. There are no headers between the sets of examples, so it looks like everything is related, even though there are billing things mixed in with scopes. Maybe not something for us to fix, but it feels like something to flag to Learn?

Copy link

Choose a reason for hiding this comment

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

+1, this page is problematic.
+1, not necessarily something we need to fix here, but would be great to rethink this page & its purpose

const data: ReferenceEntityTemplateSchema = {
name: 'Scopes',
description:
'Contains functions used to manage optional scopes for your app.\n\nThis object is returned on authenticated Admin requests.',
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
'Contains functions used to manage optional scopes for your app.\n\nThis object is returned on authenticated Admin requests.',
'Contains functions used to manage scopes for your app.\n\nThis object is returned on authenticated Admin requests.',

I remember Jake making a point to say that this was the "scopes api", not the "optional scopes api". What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate question: how are scopes managed for the storefront API? We don't want optional scopes for the storefront API, right?

*}
* ```
* ```ts
* // /app/routes/**\/*.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this file provide any value to the example? Looking at the docs on your Spin instance, It took me a minute to realize that this was a button to post at /query

*
* const body = await request.formData();
* const scopesToRevokeData = body.getAll("scopes") as string[];
* const scopesToRevoke = new AuthScopes(scopesToRevokeData);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the description, we call out that developers should implement their own validation to make sure the scopes are valid before passing them into scopes.revoke. Is new AuthScopes(...) doing this validation? Should we explicitly call it out in a comment like // Ensure scopes provided are valid or something?

Copy link

@kbav kbav left a comment

Choose a reason for hiding this comment

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

Love seeing this and all the work you've put into it, Ryan! Thank you for working on this 🙌

I'm in process of reviewing this and I have a dumb question... but where does the code for the examples on the right live?

For example, for the query method, I was going to propose a few changes

  • rather than set up a Resource Route we POST to for retrieving data through action, we could show using a Resource Route for retrieving data through loader. The useRouteLoaderData hook can be used for scenarios like this to then pull in data from other routes
  • request method example was a bit convoluted (may be clearer to just have an action and button on the same page, instead of using a Resource Route) and not clear that it leads to a full page redirect.

I was about to write "similar to the above" for the revoke method but now I'm wondering if it would just be overall better to demonstrate these being used in loaders and actions right on the same page. It’s a bit less mental overhead for looking at the examples, and would change the examples from having 3 tabs/files to 2 tabs/files — importantly, with all the relevant details in the first/visible tab.

What do you think about that suggestion?

@@ -32,6 +32,7 @@ const data: ReferenceEntityTemplateSchema = {
'EmbeddedAdminContext',
'AdminApiContext',
'BillingContext',
'ScopesApiContext',
Copy link

Choose a reason for hiding this comment

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

+1, this page is problematic.
+1, not necessarily something we need to fix here, but would be great to rethink this page & its purpose

@@ -166,6 +166,7 @@ const data: LandingTemplateSchema = {
"\n\nIt takes advantage of [Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation)" +
" to handle automatic app installations and scope updates, while using" +
" [token exchange](https://shopify.dev/docs/apps/auth/get-access-tokens/token-exchange) to get an access token for the logged-in user." +
"\n\n If you wish to learn about Scopes Management and APIs, please read through [Managing Access Scopes](https://shopify.dev/docs/apps/build/authentication-authorization/app-installation/managing-access-scopes)" +
Copy link

Choose a reason for hiding this comment

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

Suggested change
"\n\n If you wish to learn about Scopes Management and APIs, please read through [Managing Access Scopes](https://shopify.dev/docs/apps/build/authentication-authorization/app-installation/managing-access-scopes)" +
"\n\n If you wish to learn about scopes management and APIs, please read through [Manage access scopes](https://shopify.dev/docs/apps/build/authentication-authorization/app-installation/managing-access-scopes)" +

const data: ReferenceEntityTemplateSchema = {
name: 'Scopes',
description:
'Contains functions used to manage scopes for your app.\n\nThis object is returned on authenticated Admin requests.',
Copy link

Choose a reason for hiding this comment

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

Suggested change
'Contains functions used to manage scopes for your app.\n\nThis object is returned on authenticated Admin requests.',
'Contains methods used to manage scopes for your app.\n\nThis object is returned on authenticated Admin requests.',

*
* @example
* <caption>Request consent from the merchant to grant the provided scopes for this app.</caption>
* <description>Call `scopes.request` to get request optional scopes.</description>
Copy link

Choose a reason for hiding this comment

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

Suggested change
* <description>Call `scopes.request` to get request optional scopes.</description>
* <description>Call `scopes.request` to request optional scopes.</description>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants