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] Unauthenticated admin client #407

Closed

Conversation

byrichardpowell
Copy link
Contributor

WHY are these changes introduced?

We've had multiple requests to be able to instantiate admin clients outside of requests from Shopify.

WHAT is this pull request doing?

This PR introduces a new API:

const shopify = shopifyApp(config)
shopify.unauthenticated.admin(shop)

This allows an app to get a shop from whatever means , and then get an Admin API client for that Request. Possible use cases:

  1. Background jobs
  2. External Requests from non Shopify surfaces.

For example:

import shopify from "../shopify.server.js"
import {customAuthentication} from "../helpers"

export loader({request}) {
   const {shop} = customAuthentication(request)
   const {admin} = shopify.unauthenticated.admin(shop)
}

Type of change

  • 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 yarn 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)

@byrichardpowell byrichardpowell requested a review from a team as a code owner August 18, 2023 19:36
@byrichardpowell byrichardpowell changed the base branch from main to feature_react_exports August 18, 2023 19:36
@byrichardpowell
Copy link
Contributor Author

@paulomarg I think we made a mistake in the previous API design. Observe the differences here:

Webhook:

const { admin } = await authenticate.webhook(request);

admin.rest.get
admin.rest.AbandonedCheckout

Admin:

const { admin } = await authenticate.admin(request);

admin.rest.get;
admin.rest.resources.AbandonedCheckout;

I believe long term we want admin.rest.resources.AbandonedCheckout everywhere, but that would be a breaking change. I'm thinking we log the issue as a V2 change, and accept the inconsistency for now?

@paulomarg
Copy link
Contributor

paulomarg commented Aug 21, 2023

I believe long term we want admin.rest.resources.AbandonedCheckout everywhere, but that would be a breaking change. I'm thinking we log the issue as a V2 change, and accept the inconsistency for now?

Dang, you're right - we'd want resources.<...> everywhere. Since it'd be a reference assignment, could we have both webhook.rest and webhook.rest.resources point to the same collection of resources? I think the extra memory cost would be just that variable, and we could be prepared for an eventual removal in V2.

Not sure if that would work because there are more things under webhook.rest, though. We'd need to replicate the other bits of the API there as well, which isn't great. Might be worth biting the bullet and just having the resources in both places (maybe with a deprecation warning?), so folks can start migrating sooner rather than later?

Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

I know it's WIP, but just wanted to add some thoughts :) - LGTM so far!

expect(() => shopifyApp(config)).toThrowError(ShopifyError);
});

describe('unauthenticated admin context', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should at some point consider breaking this out of shopify-app, and also creating a suite of tests for the admin context that takes in the context object and runs the tests on it to prevent regressions on one of the surfaces that use it.

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.

2 participants