Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: support custom APIs @W-15111169@ #149
Changes from 16 commits
c347cdb
f640715
5422ecb
e14fa98
afe02d3
b570a91
fef40ae
04e4ae1
dd26eae
55afa1d
2ee3c0e
b646330
48dcd28
ef6033a
d73dfab
237e7cc
f7ab768
944a273
0b0e5fb
efc1863
3522f36
88f58f5
c9579bd
5fb1e17
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
calledcustomApiPathParameters
path parameters can now be passed either in
options.customApiPathParameters
orclientConfig.parameters
.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
?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:
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:
and updated README example to reflect this new change and provide a mutation request example
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"
ifContent-Type
header is not providedThere 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
Behavior is documented in the tsdoc comments. When we release and run the generate doc command it'll populate the static docs site:
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?