Fix: Isomorphic OAS SDK gaps (@W-18669767@)#197
Conversation
| @@ -25,14 +25,12 @@ export enum {{classname}}{{enumName}} { | |||
| {{/allowableValues}} | |||
| } | |||
| {{/stringEnums}}{{^stringEnums}} | |||
There was a problem hiding this comment.
The resulting type is functionally the same, we just remove the exported variable. Example:
Before:
export const BasketChannelTypeEnum = {
Storefront: 'storefront',
Callcenter: 'callcenter',
Marketplace: 'marketplace',
Dss: 'dss',
Store: 'store',
Pinterest: 'pinterest',
Twitter: 'twitter',
Facebookads: 'facebookads',
Subscriptions: 'subscriptions',
Onlinereservation: 'onlinereservation',
Customerservicecenter: 'customerservicecenter',
Instagramcommerce: 'instagramcommerce',
Tiktok: 'tiktok',
Snapchat: 'snapchat',
Google: 'google',
Whatsapp: 'whatsapp',
Youtube: 'youtube'
} as const;
// type BasketChannelTypeEnum = "storefront" | "callcenter" | "marketplace" | "dss" | "store" | "pinterest" | "twitter" | "facebookads" | "subscriptions" | "onlinereservation" | "customerservicecenter" | ... 5 more ... | "youtube"
export type BasketChannelTypeEnum = typeof BasketChannelTypeEnum[keyof typeof BasketChannelTypeEnum];After:
export type BasketChannelTypeEnum =
'storefront'|
'callcenter'|
'marketplace'|
'dss'|
'store'|
'pinterest'|
'twitter'|
'facebookads'|
'subscriptions'|
'onlinereservation'|
'customerservicecenter'|
'instagramcommerce'|
'tiktok'|
'snapchat'|
'google'|
'whatsapp'|
'youtube';|
|
||
| // There should be no code_challenge for private client | ||
| const expectedReqOptions = { | ||
| accessToken: "access_token", |
There was a problem hiding this comment.
heads up that I applied a fix to this test also as part of #198 .
In there, I replaced expectedReqOptions with a check that ensures code_challenge wasn't included.
vcua-mobify
left a comment
There was a problem hiding this comment.
I noted a question but the PR looks good to me.
We might have a conflict with the slasHelper test fixes with https://github.com/SalesforceCommerceCloud/commerce-sdk-isomorphic/pull/198/files though. Could you apply the fix from there?
| TokenResponse, | ||
| } from '../../lib/shopperLogin'; | ||
| } from '../../lib/shopperLogin/apis'; | ||
| import {TokenResponse} from '../../lib/shopperLogin/models'; |
There was a problem hiding this comment.
We went over this in the meet earlier but could you restate why this change in particular is needed or how the original version resulted in a conflict in the rolled up typescript?
There was a problem hiding this comment.
Yeah, when we imported directly from the top level index file, it would duplicate the namespace ShopperLoginTypes in the generated *.d.ts file
By importing from the apis/models directory directly, we circumvent this issue
| * | ||
| */ | ||
| export class {{#lambda.titlecase}}{{#lambda.camelcase}}{{appName}}{{/lambda.camelcase}}{{/lambda.titlecase}}<ConfigParameters extends {{#lambda.titlecase}}{{#lambda.camelcase}}{{appName}}{{/lambda.camelcase}}{{/lambda.titlecase}}Parameters & Record<string, unknown>> { | ||
| export class {{#vendorExtensions}}{{#x-sdk-classname}}{{{ . }}}{{/x-sdk-classname}}{{^x-sdk-classname}}{{#lambda.titlecase}}{{#lambda.camelcase}}{{appName}}{{/lambda.camelcase}}{{/lambda.titlecase}}{{/x-sdk-classname}}{{/vendorExtensions}}{{^vendorExtensions}}{{#lambda.titlecase}}{{#lambda.camelcase}}{{appName}}{{/lambda.camelcase}}{{/lambda.titlecase}}{{/vendorExtensions}}<ConfigParameters extends {{#lambda.titlecase}}{{#lambda.camelcase}}{{appName}}{{/lambda.camelcase}}{{/lambda.titlecase}}Parameters & Record<string, unknown>> { |
There was a problem hiding this comment.
Thanks for this; I've applied the template changes (with some slight tweaks) to the node SDK PR: SalesforceCommerceCloud/commerce-sdk#418
The tweaks were to account for the namespacing that we do in the node sdk.
In the future I wonder if there is some way we can make this easier to read or some way to extract it so we only define the full vendorExtensions block once. Not a blocker for this PR though.
unandyala
left a comment
There was a problem hiding this comment.
I see the issues with the generated ShopperContexts classname and type
export class {classname=ShopperContexts}
export namespace {classname=ShopperContexts}Types {
other than that the changes look good to me
@unandyala could you clarify where you see this issue? |
This PR fixes the gaps in OAS SDK generation, namely the typescript naming collisions in the generated
index.cjs.d.tsandindex.esm.d.tsfiles, and adds support for implementing a custom class name for Shopper Context via the custom vendor extension,x-sdk-classname.This PR also does the following:
Problem
There are several reasons why the naming collisions occur.
rollup/rollup-plugin-tsis good about name collisions between types, and will append extra characters to prevent naming collisions. Example:However, there are certain cases where rollup isn't able to resolve these naming collisions, such as for namespaces and variables.
Solution
There are several ways we solve the naming collision issues:
ApiTypesandModelTypes, so we append the classname to those, such asShopperCustomersApiTypesindex.tsfile caused the generated*.d.tsfile to duplicate types, we just had to fix the way we imported here (templates/index.ts.hbs)ShopperLoginis exported twice due to the SLAS helpers files due to the usage ofTokenResponsetype and creating a type from theShopperLoginclass. The fix was to create an interface that we use for typing instead. We do this in the commerce SDK: https://github.com/SalesforceCommerceCloud/commerce-sdk/blob/1c729a9a93af9bde5aea9a4a760be6e2d1533b19/src/static/helpers/slasClient.ts#L47