-
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: Add support for custom query parameters @W-13974911@ #139
Conversation
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.
Thanks @joeluong-sfcc – I ended up coming the behaviour by running HTTP requests through a MITM proxy:
// clear ; NODE_EXTRA_CA_CERTS=~/.mitmproxy/mitmproxy-ca-cert.cer node index.js
import csi from "commerce-sdk-isomorphic";
const { helpers, ShopperLogin, ShopperProducts, ShopperBaskets } = csi;
import hpa from "https-proxy-agent";
const { HttpsProxyAgent } = hpa;
const redirectURI = "http://localhost:3000/callback";
const clientConfig = {
parameters: {
clientId: "",
organizationId: "",
shortCode: "",
siteId: "",
},
fetchOptions: {
agent: new HttpsProxyAgent("http://localhost:3001"),
},
};
const guestTokenResponse = await helpers.loginGuestUser(
new ShopperLogin(clientConfig),
{
redirectURI,
}
);
const productsResult = await new ShopperProducts({
...clientConfig,
headers: { authorization: `Bearer ${guestTokenResponse.access_token}` },
}).getProducts({
parameters: {
ids: "25720044M",
c_custom: "custom",
invalid: "123",
},
});
console.log("productsResult: ", productsResult);
I can see that the outgoing requests includes the expected c_custom
param. Yah!
it('allow custom query params', async () => { | ||
const customersClient = new ShopperCustomers({ | ||
parameters: { | ||
// shortCode is a base URI parameter, not path/query, so it *must* be in the config | ||
shortCode: SHORT_CODE, | ||
}, | ||
}); | ||
|
||
const options = { | ||
parameters: { | ||
siteId: SITE_ID, | ||
organizationId: ORGANIZATION_ID, | ||
clientId: CLIENT_ID, | ||
c_validCustomParam: 'custom_param', | ||
invalidParam: 'invalid_param', | ||
}, | ||
body: {type: 'guest'}, | ||
}; | ||
|
||
nock(`https://${SHORT_CODE}.api.commercecloud.salesforce.com`) | ||
.post( | ||
`/customer/shopper-customers/v1/organizations/${ORGANIZATION_ID}/customers/actions/login` | ||
) | ||
.query({ | ||
siteId: SITE_ID, | ||
clientId: CLIENT_ID, | ||
// expect `c_validCustomParam` but not `invalidParam` | ||
c_validCustomParam: 'custom_param', | ||
}) | ||
.reply(200, MOCK_RESPONSE); | ||
|
||
const response = await customersClient.authorizeCustomer(options); | ||
|
||
expect(response).toEqual(MOCK_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.
Nice test case!
.github/workflows/test.yml
Outdated
@@ -9,7 +9,7 @@ jobs: | |||
linux-tests: | |||
strategy: | |||
matrix: | |||
node: [10, 12, 14, 16] | |||
node: [10, 12, 14, 16, 18] |
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.
Given that we've tentatively merged Node 20 support into PWA Kit @ 3.4, would you consider adding Node 20 here?
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 see an error when running yarn install
using Node 18 or 20:
warning Error running install script for optional dependency: "/Users/arayanavarro/git/commerce-sdk-isomorphic/node_modules/watchpack-chokidar2/node_modules/fsevents, /Users/arayanavarro/git/commerce-sdk-isomorphic/node_modules/webpack-dev-server/node_modules/fsevents: Command failed.
Exit code: 1
Command: node install.js
Arguments:
Directory: /Users/arayanavarro/git/commerce-sdk-isomorphic/node_modules/webpack-dev-server/node_modules/fsevents
Output:
node:events:495
throw er; // Unhandled 'error' event
^
Error: spawn node-gyp ENOENT
at ChildProcess._handle.onexit (node:internal/child_process:284:19)
at onErrorNT (node:internal/child_process:477:16)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
Emitted 'error' event on ChildProcess instance at:
at ChildProcess._handle.onexit (node:internal/child_process:290:12)
at onErrorNT (node:internal/child_process:477:16)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
errno: -2,
code: 'ENOENT',
syscall: 'spawn node-gyp',
path: 'node-gyp',
spawnargs: [ 'rebuild' ]
[-/10] ⡀ waiting...
[2/10] ⡀ iltorb
[-/10] ⡀ waiting...
[4/10] ⡀ husky
error /Users/arayanavarro/git/commerce-sdk-isomorphic/node_modules/iltorb: Command failed.
Exit code: 127
Command: node ./scripts/install.js || node-gyp rebuild
Arguments:
Directory: /Users/arayanavarro/git/commerce-sdk-isomorphic/node_modules/iltorb
Output:
info looking for cached prebuild @ /Users/arayanavarro/.npm/_prebuilds/efa18a-iltorb-v2.4.5-node-v108-darwin-x64.tar.gz
http request GET https://github.com/nstepien/iltorb/releases/download/v2.4.5/iltorb-v2.4.5-node-v108-darwin-x64.tar.gz
Do we want to also include that info in the README?
commerce-sdk-isomorphic/README.md
Line 11 in 580fc61
- Node `^12.x`, `^14.x`, or `^16.x` |
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've added node 20 to the CI and updated the README
to include Node ^18.x
.
@adamraya As for the yarn install
error, this doesn't appear on my machine and the CI seems to be able to run yarn install
successfully for both node 18 and 20: https://github.com/SalesforceCommerceCloud/commerce-sdk-isomorphic/actions/runs/7546319904/job/20543824549
What version of yarn
do you have installed on your machine? Have you tried deleting node_modules
and yarn.lock
and then running yarn install
?
templates/operations.ts.hbs
Outdated
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 can only guess at what we're doing here, but looks right 😅
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.
If you run yarn run renderTemplates
and then check the output directory (src/lib/
), you can see the rendered typescript, probably easier to look at then the handlebar template
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 followed the test steps and confirmed that the custom query parameters beginning with the prefix "c_" are included in the request, while the remaining query parameters are ignored.
But, I did notice an issue related to the addition of the new Node version https://github.com/SalesforceCommerceCloud/commerce-sdk-isomorphic/pull/139/files#r1451043899
87394e2
Co-authored-by: Adam Raya <[email protected]>
This PR adds support for custom query parameters as SCAPI now supports SCAPI hooks: https://developer.salesforce.com/docs/commerce/commerce-api/references/about-commerce-api/about.html#08302023
Query params that begin with
c_
are considered custom query parameters and are passed onto the underlying SCAPI call. Any other query params that aren't a part of the SCAPI endpoint spec will be filtered out and not passed.This PR also adds node 18 & 20 to the CI
Testing this PR takes a bit of set up:
git clone [email protected]:SalesforceCommerceCloud/commerce-sdk-isomorphic.git
git checkout ju/support-custom-params
yarn install
templates/operations.ts.hbs
to add aconsole.log()
to check query params:yarn run renderTemplates && yarn run build:lib
mkdir testCustomParams cd testCustomParams npm init -y touch index.js
package.json
intestCustomParams
directory to pointcommerce-sdk-isomorphic
to local directory independencies
section and add"type": "module"
:npm install
index.js
and replace client credsnode index.js > out.txt
Observe
out.txt
and that the custom query params are included in the fetch call and invalid query params are excluded from the call: