-
Notifications
You must be signed in to change notification settings - Fork 22
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
Feat: support custom APIs @W-15111169@ #149
Conversation
templates/operations.ts.hbs
Outdated
headers, | ||
method: "{{loud method}}" | ||
}; | ||
const response = await runFetchHelper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were their any measurable change in the side of a generated lib before and after with this change. Looks like we are only saving 2 lines roughly per use of this template, but it might add up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn run check:size
for main
branch:
PASS lib/index.cjs.js: 44.84KB < maxSize 45KB (gzip)
PASS lib/index.esm.js: 44.73KB < maxSize 45KB (gzip)
PASS commerce-sdk-isomorphic-with-deps.tgz: 337.06KB < maxSize 350KB (gzip)
yarn run check:size
for ju/custom-endpoint
branch:
PASS lib/index.cjs.js: 44.13KB < maxSize 45KB (gzip)
PASS lib/index.esm.js: 44.02KB < maxSize 45KB (gzip)
PASS commerce-sdk-isomorphic-with-deps.tgz: 337.43KB < maxSize 350KB (gzip)
Seems like lib/index.cjs.js
and lib/index.esm.js
have decreased slightly, but commerce-sdk-isomorphic-with-deps.tgz
has increased slightly 😮
README.md
Outdated
endpointPath: 'customers', | ||
apiName: 'loyalty-info', | ||
apiVersion: 'v1', // defaults to v1 if not provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientConfig
normally contains a static values – clientId|organizationId|shortCode|siteId
– these things rarely if ever change between calls in regular implementations.
endpointPath|apiName|apiVersion
are likely to change.
Would you consider moving these values into the options
of the request? At least to me, if feels more natural to have them there with other options that usually vary call to call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new property to options
called customApiPathParameters
customApiPathParameters?: {
apiName?: string;
apiVersion?: string;
endpointPath?: string;
organizationId?: string;
shortCode?: string;
};
path parameters can now be passed either in options.customApiPathParameters
or clientConfig.parameters
.
README.md
Outdated
const rawResponse = false; | ||
const accessToken = '<INSERT ACCESS TOKEN HERE>'; | ||
|
||
let response = await helpers.callCustomEndpoint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly, but would you consider an alternative argument structure for callCustomEndpoint
?
await helpers.callCustomEndpoint({
options: myOptions,
clientConfig: myClientConfig,
rawResponse: true
})
This interface makes it much more clear to folks using regular old JavaScript what the impact of the three arguments will be.
It would also allow you to significantly shrink this code example:
await helpers.callCustomEndpoint({
options: {
method: "GET",
parameters: {
queryParameter: "queryParameter1",
siteId: SITE_ID,
},
headers: {
"Content-Type": "application/json",
authorization: `Bearer <your-access-token>`,
},
},
clientConfig: {
clientId: "<your-client-id>",
organizationId: "<your-org-id>",
shortCode: "<your-short-code>",
siteId: "<your-site-id>",
// Custom API path parameters
endpointPath: "customers",
apiName: "loyalty-info",
apiVersion: "v1", // defaults to v1 if not provided
},
rawResponse: true,
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you also consider showing an example of a mutation request eg. a POST where you pass a request body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated argument structure to be a single argument, following the pattern provided above:
await helpers.callCustomEndpoint({
options: myOptions,
clientConfig: myClientConfig,
rawResponse: true
})
and updated README example to reflect this new change and provide a mutation request example
siteId: SITE_ID, | ||
}, | ||
headers: { | ||
'Content-Type': 'application/json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect callers to pass the content type or can it be automatically inferred or default to JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated implementation to default to "Content-Type" : "application/json"
if Content-Type
header is not provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joeluong-sfcc – the updated interface looks good. I've only reviewed the README, I'll let the rest up to others.
README.md
Outdated
const CLIENT_ID = "<your-client-id>"; | ||
const ORG_ID = "<your-org-id>"; | ||
const SHORT_CODE = "<your-short-code>"; | ||
const SITE_ID = "<your-site-id>"; | ||
|
||
// client configuration parameters | ||
const clientConfigExample = { | ||
parameters: { | ||
clientId: CLIENT_ID, | ||
organizationId: ORG_ID, | ||
shortCode: SHORT_CODE, | ||
siteId: SITE_ID, | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly, but you're repeating yourself here:
const CLIENT_ID = "<your-client-id>"; | |
const ORG_ID = "<your-org-id>"; | |
const SHORT_CODE = "<your-short-code>"; | |
const SITE_ID = "<your-site-id>"; | |
// client configuration parameters | |
const clientConfigExample = { | |
parameters: { | |
clientId: CLIENT_ID, | |
organizationId: ORG_ID, | |
shortCode: SHORT_CODE, | |
siteId: SITE_ID, | |
} | |
}; | |
const clientConfigExample = { | |
parameters: { | |
clientId: <your-client-id>, | |
organizationId: "<your-org-id>", | |
shortCode: "<your-short-code>", | |
siteId: "<your-site-id>", | |
} | |
}; |
Also, because you've chosen meaningful variable names, you don't really need the code comment.
Why do you pass siteId
as a param to the client Config and then pass it again as a parameter
later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a miss where I didn't use the siteId from the clientConfig, code has been updated to use siteId as query parameter if available
src/static/helpers/customApi.ts
Outdated
pathParams.apiVersion = 'v1'; | ||
} | ||
|
||
const defaultBaseUri = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We previously talked about this decision on hard coding prod uri.. are we cool with that. Have we documented it anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the default base URI if you don't provide one yourself. You can override this behavior by passing in your own baseUri in the clientConfig
const clientConfig = {
parameters: {
...
},
baseUri: 'https://{shortCode}.alternativeBaseUri.com/custom/{apiName}/{apiVersion}'
};
Behavior is documented in the tsdoc comments. When we release and run the generate doc command it'll populate the static docs site:
* @param args.clientConfig.baseUri? - baseUri used for the request, where the path parameters are wrapped in curly braces. Default value is 'https://{shortCode}.api.commercecloud.salesforce.com/custom/{apiName}/{apiVersion}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my question is, and I think I know the answer. But if you were to generate a client where the raml specifies the base url a something like 'https://{shortCode}.dev-api.commercecloud.salesforce.com/custom/{apiName}/{apiVersion}' notice the "dev" in "dev-api" all calls to the apis will goto that url. but if I make a custom api call without passing that "dev" value, it will make it to production. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the callCustomEndpoint
helper function does not interact with any type of RAML, it'll use the production URI as default if you don't provide a base URI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bendvc I think you are right. Unless the user overrides baseUri it will always go to the default. @joeluong-sfcc Can we read the default value from a config file instead of hard-coding so that it is easy to change the default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be we shouldn't have default URI. How about just throwing an error if the baseUri is not provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the default base URI into a config file. I think it still makes sense to have a default URI as all the other APIs use the production URI as default, but I've made it more clear in the README example on how to pass it into the function. Does that work for y'all @unandyala @bendvc?
} | ||
{{/unless}} | ||
{{else}} | ||
return response as Response | {{getReturnTypeFromOperation this}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this line a little bit more for me. If we are expecting that we aren't using a rawResponse we are saying that the return value is the union of a Response object and the return type of this operation? Why wouldn't it simply be the return type of the operation without the union?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the rawResponse
when we call the doFetch
helper function for the response
variable. Since we don't know what rawResponse
is when we pass it to doFetch
, the result is either a response object OR the return type as doFetch
will either return a raw response object or the return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is to handle any unexpected response - the response that is not defined in RAML specially in case of error response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds support for custom APIs by exposing a helper function called
callCustomEndpoint
that allows users to call their custom API following common SDK patterns. This PR also pulls out some of the reused logic inoperations.ts.hbs
for fetching data and puts into a helper function calleddoFetch
.Some features of the
callCustomEndpoint
helper:apiVersion
tov1
if not providedcontent-type
header toapplication/json
if not providedBelow is a sample script I used for testing the
callCustomEndpoint
helper function. Reach out to me for instructions on how to run this with the proper credentials.Resulting fetch request URLs:
https://shortcode.api.commercecloud.salesforce.com/custom/e2e-tests/v1/organizations/orgId/greeting?siteId=siteId
http://localhost:8888/mobify/proxy/api/custom/e2e-tests/v1/organizations/orgId/greeting?siteId=siteId
https://shortcode.alternativebaseuri.com/custom/e2e-tests/v1/organizations/orgId/greeting?siteId=siteId