@W-18575180@ Generate node SDK from OAS#418
@W-18575180@ Generate node SDK from OAS#418vcua-mobify merged 50 commits intoSalesforceCommerceCloud:feature/oasfrom
Conversation
|
I was doing some file clean up on the fork and it somehow closed the PR =/ |
| ) { | ||
| return { | ||
| filepath: path.join(directory, exchangeConfig.main), | ||
| filename: exchangeConfig.main, |
There was a problem hiding this comment.
Does changing the filename to include V2 change the class name for the generated class? If so I think we should do that
There was a problem hiding this comment.
This refers to the file name of the file that's defined in exchange.json. For shopper baskets v2, this is shopper-baskets-oas-v2-bundled.yaml
If we change the name of the .yaml file then we will need to come up with a different way to get that file name rather than relying on exchange.json.
joeluong-sfcc
left a comment
There was a problem hiding this comment.
Will you create a PR for updating APIs?
I also noticed that I can't seem to get my local RAML toolkit directory linked up to the node SDK without using yarn, once we release the RAML toolkit with the new changes, this should be fine. If we aren't able to, however, we'll have a problem
| "snyk": "^1.836.0", | ||
| "snyk": "^1.996.0", | ||
| "sort-package-json": "^1.53.1", | ||
| "source-map-support": "^0.5.21", |
There was a problem hiding this comment.
This is required to be able to run tests with npm. Not sure why this wasn't required when I was running the tests through yarn.
| "eslint-plugin-tsdoc": "^0.2.17", | ||
| "fs-extra": "^9.1.0", | ||
| "handlebars-helpers": "^0.10.0", | ||
| "handlebars": "4.7.7", |
There was a problem hiding this comment.
depcheck wants this dependency to be handlebars rather than handlebars-helpers.
I've set the version of handlebars to the same version used by the isomorphic sdk
| "test:prod": "npm run bundle:install && npm run test", | ||
| "test:staging": "COMMERCE_SDK_INPUT_DIR=testIntegration/stagingApis npm run test:prod", | ||
| "test:ci": "npm run test:prod && npm run test:staging", | ||
| "test:ci": "npm run test:prod", |
There was a problem hiding this comment.
The tests already run against the apis downloaded to the apis directory so I don't see why we need to duplicate the download to the staging directory + test run.
Note: This may change in the near future if we start making a distinction between 'external' and 'internal' apis.
| expect(nock.isDone()).to.be.true; | ||
| }); | ||
| // TODO: Investigate why this test fails if a user-agent header is passed in the method call | ||
| // it("merges with alternative case header in method", async () => { |
There was a problem hiding this comment.
I'm not sure why this test in particular is failing. The API includes a call to mergeHeaders like before:
const headers = mergeHeaders(getHeaders(options), {
[USER_AGENT_HEADER]: USER_AGENT_VALUE
});
so I'm not sure if something has changed with the implementation in commerce-apps-core
There was a problem hiding this comment.
+1 to Ujwala's point for documenting this work, we should create a small ticket or update one of our other tickets to revisit this so we don't lose track of it
unandyala
left a comment
There was a problem hiding this comment.
Overall LGTM. If there are known issues, please create follow up tickets to fix
| "catalogs-oas", | ||
| "cdn-api-process-apis-oas", | ||
| "coupons-oas", | ||
| // "cors-preferences-oas", // This is included in preferences-oas. Error: diff failed: duplicate endpoint (PUT /organizations/{organizationId}/cors) found in apis/cors-preferences-oas/cors-preferences-oas/cors-preferences-oas-v1-bundled.yaml and apis/preferences-oas/cors-preferences-oas/cors-preferences-oas-v1-bundled.yaml. You may add the x-since-date extension to specify order |
There was a problem hiding this comment.
so we don't need this API family? If so can we delete from here?
| "customers-oas", | ||
| "gift-certificates-oas", | ||
| "orders-oas", // Error: failed to load base specs from glob "updateApisTmp/oldApis/**/*.yaml": failed to load "updateApisTmp/oldApis/orders-oas/orders-oas/orders-oas-v1-bundled.yaml": failed to unmarshal data: json error: invalid character 'o' looking for beginning of value, yaml error: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal array into field Schema.items of type openapi3.Schema | ||
| "products-oas", // Error: diff failed: duplicate endpoint (GET /organizations/{organizationId}/products/{id}) found in apis/products-oas/products-oas/products-oas-v1-bundled.yaml and apis/products-oas/products-oas-test/products-oas-test-v1-bundled.yaml. You may add the x-since-date extension to specify order |
There was a problem hiding this comment.
Are we downloading both products-oas-v1-bundled.yaml and products-oas-test-v1-bundled.yaml? Is this something we need to fix in the download?
There was a problem hiding this comment.
This issue was fixed once I started downloading the latest stable version rather than a snapshot.
I'll remove the comments
| // "cors-preferences-oas", // This is included in preferences-oas. Error: diff failed: duplicate endpoint (PUT /organizations/{organizationId}/cors) found in apis/cors-preferences-oas/cors-preferences-oas/cors-preferences-oas-v1-bundled.yaml and apis/preferences-oas/cors-preferences-oas/cors-preferences-oas-v1-bundled.yaml. You may add the x-since-date extension to specify order | ||
| "customers-oas", | ||
| "gift-certificates-oas", | ||
| "orders-oas", // Error: failed to load base specs from glob "updateApisTmp/oldApis/**/*.yaml": failed to load "updateApisTmp/oldApis/orders-oas/orders-oas/orders-oas-v1-bundled.yaml": failed to unmarshal data: json error: invalid character 'o' looking for beginning of value, yaml error: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal array into field Schema.items of type openapi3.Schema |
There was a problem hiding this comment.
Do we still have this issue? Do we need a story to fix this?
There was a problem hiding this comment.
This issue was fixed once I started downloading the latest stable version rather than a snapshot.
I'll remove the comments
joeluong-sfcc
left a comment
There was a problem hiding this comment.
Just minor comments, but otherwise LGTM
| }); | ||
| await client.authorizeCustomer({ | ||
| body: {}, | ||
| parameters: params, |
There was a problem hiding this comment.
Just to clarify this change, we're now using authorizeCustomer from ShopperLogin instead of from ShopperCustomers and it expects a parameters object instead of a request body?
| const tokenResponse = await slasHelpers.loginGuestUser(slasClient, { | ||
| redirectURI: "redirect_uri", | ||
| }); | ||
| // TODO: Using 'as any' to avoid type errors betwen the enum 'code' and the string 'code'. This should be fixed in the future. |
There was a problem hiding this comment.
I think we can add this as an AC to the refactor node SDK ticket (@W-18136408@) or we can create a new ticket for fixing all the tests
| `category:"CC API Family" = "${apiFamily}"`, | ||
| deployment | ||
| `"${apiFamily}" category:Visibility = "External" category:"SDK Type" = "Commerce"`, | ||
| undefined, |
There was a problem hiding this comment.
[NIT] can we add a small comment on why we passed undefined as it's not immediately clear unless you understand the search method
|
Thanks for the inputs @unandyala and @joeluong-sfcc . I'll merge this PR and will open a follow up PR shortly to address the remaining comments and get the contents of the |
9dee573
into
SalesforceCommerceCloud:feature/oas
THIS PR CONTAINS BREAKING CHANGES
This PR updates commerce-sdk so that it downloads OAS files from anypoint exchange rather than raml files
This requires the changes from SalesforceCommerceCloud/raml-toolkit#228 to work
Testing the changes:
Check out both this PR and the @W-17701650@ Add support for OAS raml-toolkit#228
Build raml-toolkit: npm install && npm run build
In commerce-sdk's package.json, ensure the dependency to raml-toolkit is pointed to your local raml-toolkit.
Run
yarn cito install dependencies and include the local raml-toolkitSet some env vars for the anypoint exchange credentials
export ANYPOINT_USERNAME="<insert_username_here>"
export ANYPOINT_PASSWORD="<insert_password_here>"
Download the OAS files from anypoint exchange and run diff check. You currently need to run this twice; the 1st run will compare the current contents of the apis dir (which contain raml files) with the newly downloaded oas files. The 2nd run onwards will compare only oas files (the files you downloaded from the 1st run with the files you downloaded from the 2nd run). You may also need to delete the
updateApisTempdirectory.yarn updateApisyarn renderTemplateswill call raml-toolkit to run openapi-generator. The files it generates will be outputted underrenderedTemplatesCurrently the generated files have a naming conflict for ShopperContexts, CorsPreferences, Catalog, and ShopperBasketsV2. Current workaround is to manually address the name conflicts or to edit the oas files in question that were downloaded from exchange and define a
x-sdk-classnameFor example:
For the other affected APIs, set the
x-sdk-classnameto one of:CORSPreferencesorShopperContextsorShopperBasketsV2Bundle the generated files with
yarn compileRun tests with
yarn testGenerate docs with
yarn docNote:
The changes from SalesforceCommerceCloud/commerce-sdk-isomorphic#197 have been applied to the mustache templates here.
TODO - will be done in a follow up PR:
OASDiff fails with duplicate endpoint for order/shopper-order and product/shopper-product - this will be a separate ticket
Investigate why doc generation has many duplicate typesthis will be a separate ticket