[WIP][Admin API Client] Add Admin API client package #1022
Conversation
scottdixon
force-pushed
the
sd-admin-client
branch
4 times, most recently
from
4537ee8 to
fa9acee
Compare
scottdixon
force-pushed
the
sd-admin-client
branch
from
fa9acee to
d9578b4
Compare
| "User-Agent": `${ | ||
| userAgentPrefix ? `${userAgentPrefix} | ` : "" | ||
| }${CLIENT} v${DEFAULT_CLIENT_VERSION}`, |
There was a problem hiding this comment.
This is a mouthful 😅 is there a cleaner approach?
There was a problem hiding this comment.
Can't think of anything other than pulling the prefix part into its own variable, but that doesn't really change anything. Fine as is IMO.
| "Content-Type": "application/json", | ||
| Accept: "application/json", | ||
| "X-Shopify-Access-Token": "access-token", | ||
| "User-Agent": "Admin API Client vROLLUP_REPLACE_CLIENT_VERSION", |
There was a problem hiding this comment.
Is there some way to test it's actually replacing ROLLUP_REPLACE_CLIENT_VERSION?
There was a problem hiding this comment.
Maybe importing the built file? Seems like overkill to me though, I think we can trust rollup to do its thing if we manually check it.
| "User-Agent": `${ | ||
| userAgentPrefix ? `${userAgentPrefix} | ` : "" | ||
| }${CLIENT} v${DEFAULT_CLIENT_VERSION}`, |
There was a problem hiding this comment.
Can't think of anything other than pulling the prefix part into its own variable, but that doesn't really change anything. Fine as is IMO.
| "Content-Type": "application/json", | ||
| Accept: "application/json", | ||
| "X-Shopify-Access-Token": "access-token", | ||
| "User-Agent": "Admin API Client vROLLUP_REPLACE_CLIENT_VERSION", |
There was a problem hiding this comment.
Maybe importing the built file? Seems like overkill to me though, I think we can trust rollup to do its thing if we manually check it.
scottdixon
force-pushed
the
sd-admin-client
branch
from
41bf15b to
c1624ee
Compare
| }); | ||
|
|
||
| it("throws an error when run in a browser environment (window is defined)", () => { | ||
| global.window = {} as any; |
There was a problem hiding this comment.
nit: This might not be necessary since these tests should only run on a server environment.
There was a problem hiding this comment.
Wouldn't this still be handy to catch someone removing the browser/window check?
There was a problem hiding this comment.
The test itself is fine - it's the global.window code I think we can remove.
Since these tests run only in a server environment, global.window should already be undefined.
|
|
||
| ### `createAdminApiClient()` parameters | ||
|
|
||
| | Property | Type | Description | |
There was a problem hiding this comment.
I think the README still needs to be updated regarding the new retries and logger features.
There was a problem hiding this comment.
Also, the names of the functions and types needs to be updated as well.
packages/admin-api-client/README.md
Outdated
| | -------------- | ------------------------ | ---------------------------------------------------- | | ||
| | variables? | `Record<string, any>` | Variable values needed in the graphQL operation | | ||
| | apiVersion? | `string` | The Admin API version to use in the API request | | ||
| | customHeaders? | `{[key: string]: string}` | Customized headers to be included in the API request | |
There was a problem hiding this comment.
I'm wondering about this name - should it be just headers? At this point these are the only headers the app is deliberately sending, not sure if we need to say they're custom here.
There was a problem hiding this comment.
We could definitely change this - it's actually defined in a common ApiClient level type so I can change it there and have all the API clients use the same interface.
There was a problem hiding this comment.
@scottdixon - this should be updated on main now so when you rebase and run turbo build and turbo test, you should be able to get the latest graphql-client library types.
| ".": { | ||
| "module": { | ||
| "types": "./dist/ts/index.d.ts", | ||
| "default": "./dist/index.mjs" |
There was a problem hiding this comment.
Do we want to have require entries for the CJS build? I think that would be the right setup here?
There was a problem hiding this comment.
I think that's happening a few lines down - let me know if I'm missing something!
|
I think other than the package.json stuff (that can be a follow up as long as it gets done), I'm ok with this going in too! |
scottdixon
force-pushed
the
sd-admin-client
branch
from
15ef5ca to
bb00656
Compare
WHY are these changes introduced?
Add a new admin-api-client package. Used @melissaluu's Storefront client work as the starting point.
WHAT is this pull request doing?
Add a new admin-api-client package to the repo.
Type of change
Checklist
yarn changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfile manually)