[WIP] add shopify-app-remix scopes API documentation
#1506
Conversation
Co-authored-by: Jonathan J Chang <[email protected]>
Co-authored-by: Allen Chazhoor <[email protected]>
…uery Co-authored-by: Allen Chazhoor <[email protected]>
| @@ -32,6 +32,7 @@ const data: ReferenceEntityTemplateSchema = { | |||
| 'EmbeddedAdminContext', | |||
| 'AdminApiContext', | |||
| 'BillingContext', | |||
| 'ScopesApiContext', | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
+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.', |
There was a problem hiding this comment.
| '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?
There was a problem hiding this comment.
Separate question: how are scopes managed for the storefront API? We don't want optional scopes for the storefront API, right?
.../apps/shopify-app-remix/src/server/authenticate/admin/scope/authenticate.admin.scopes.doc.ts
Outdated
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/types.ts
Outdated
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/scope/types.ts
Outdated
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/scope/types.ts
Outdated
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/scope/types.ts
Outdated
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/scope/types.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * const body = await request.formData(); | ||
| * const scopesToRevokeData = body.getAll("scopes") as string[]; | ||
| * const scopesToRevoke = new AuthScopes(scopesToRevokeData); |
There was a problem hiding this comment.
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?
…embedded app auth strat section
allenchazhoor
force-pushed
the
scopes-api-documentation
branch
from
28d879f
to
af3f849
Compare
There was a problem hiding this comment.
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 throughloader. TheuseRouteLoaderDatahook can be used for scenarios like this to then pull in data from other routes requestmethod example was a bit convoluted (may be clearer to just have anactionand 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', | |||
There was a problem hiding this comment.
+1, this page is problematic.
+1, not necessarily something we need to fix here, but would be great to rethink this page & its purpose
.../apps/shopify-app-remix/src/server/authenticate/admin/scope/authenticate.admin.scopes.doc.ts
Show resolved
Hide resolved
packages/apps/shopify-app-remix/src/server/authenticate/admin/scope/types.ts
Outdated
Show resolved
Hide resolved
|
Put up the recent changes here: https://shopify-dev.shopify-dev-3h44.allen-chazhoor.us.spin.dev/docs/api/shopify-app-remix/v3/apis/scopes |
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?
Type of change
Documentation
Checklist
pnpm changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfiles manually)