@W-18349405@ Download and generate multiple versions of ShopperBaskets#198
@W-18349405@ Download and generate multiple versions of ShopperBaskets#198vcua-mobify merged 15 commits intofeature/oasfrom
Conversation
scripts/generate-oas.ts
Outdated
|
|
||
| // Special handling for shopper-baskets-v2 | ||
| if ( | ||
| exchangeConfig.assetId === 'shopper-baskets-oas' && |
There was a problem hiding this comment.
Can we be a bit generic for all apis ? If other apis happen to have version 2, we have to come back and manually fix this?
There was a problem hiding this comment.
That's fair. We can update this so that if apiVersion is v2, then append v2 to the various names
| refreshToken: 'refresh_token', | ||
| response_type: 'code', | ||
| usid: 'usid', | ||
| const unexpectedQueryParams = { |
There was a problem hiding this comment.
Dumb question, why do we need to change this test?
There was a problem hiding this comment.
This test fix addresses a change that was made to for allowing any query parameters through: #196
Rather than check for expected options, I think it's better we assert that the code challenge wasn't included. That way, if new non-breaking options appear in the future, this test will continue to work.
scripts/utils.ts
Outdated
| const matchedApis = await download.search( | ||
| `"${name}" category:Visibility = "External" category:"SDK Type" = "Commerce" category:"SDK Type" = "Isomorphic"`, | ||
| undefined, | ||
| true |
There was a problem hiding this comment.
I assumed the search has error handling?
There was a problem hiding this comment.
This is applying a subset of the changes from #200.
It may be better for us to merge the changes from @unandyala first then add this one after
| outputDir: `${outputDir}`, | ||
| templateDir: `${TEMPLATE_DIRECTORY}`, | ||
| skipValidateSpec: true, | ||
| flags: `--reserved-words-mappings delete=delete`, |
There was a problem hiding this comment.
This is needed to address some naming collisions around the delete methods that are generated for the SDK
There was a problem hiding this comment.
What does skipValidateSpec do? Do we not want to include it alongside the new flags?
Also what naming collisions do we have with the delete keyword? Or is it just to bring it in alignment with the node SDK
There was a problem hiding this comment.
skipValidateSpec was an argument I was passing in the early days of this work to force the generator to run even when there were errors in the oas spec (back when I was pulling down snapshots rather than stable versions of the specs).
We don't need to set it now that we're downloading stable versions of the spec but the setup allows us to add it again in the future.
There was a problem hiding this comment.
can you add a comment explaining the delete flag
joeluong-sfcc
left a comment
There was a problem hiding this comment.
Correct me if I'm wrong, but it seems like the main change is we use a new helper function that recursively goes through the api directory and collects all the sub directories with an exchange.json file into an array
LGTM
| outputDir: `${outputDir}`, | ||
| templateDir: `${TEMPLATE_DIRECTORY}`, | ||
| skipValidateSpec: true, | ||
| flags: `--reserved-words-mappings delete=delete`, |
There was a problem hiding this comment.
What does skipValidateSpec do? Do we not want to include it alongside the new flags?
Also what naming collisions do we have with the delete keyword? Or is it just to bring it in alignment with the node SDK
|
@joeluong-sfcc , re: |
|
|
||
| return { | ||
| filepath: path.join(directory, exchangeConfig.main), | ||
| filepath: path.join( |
There was a problem hiding this comment.
Feels like this should be part of the search query parameter to the raml-toolkit downloader. I think it is fine for now. Can we create a followup ticket to make this change in raml-toolkit and remove this code from here?
There was a problem hiding this comment.
New ticket W-18833027 created for this suggestion
scripts/generate-oas.ts
Outdated
| } | ||
|
|
||
| export function resolveApiName(name: string, version: string): string { | ||
| if (name === 'Shopper Baskets OAS') { |
There was a problem hiding this comment.
Can we make this generic instead of hard-coding? That way any V2 api will be generated
There was a problem hiding this comment.
I've modified the function to be more generic when appending V2. But I left the special case for appending V1 to ShopperBaskets specifically for now. We'll need to remove it if ShopperBasketsV1 is reverted to just ShopperBaskets by the IPA team.
| outputDir: `${outputDir}`, | ||
| templateDir: `${TEMPLATE_DIRECTORY}`, | ||
| skipValidateSpec: true, | ||
| flags: `--reserved-words-mappings delete=delete`, |
There was a problem hiding this comment.
can you add a comment explaining the delete flag
scripts/generate-oas.ts
Outdated
| fs.writeFileSync(`${TARGET_DIRECTORY}/version.ts`, generatedVersion); | ||
| } | ||
|
|
||
| export function getAllDirectoriesWithExchangeFiles( |
There was a problem hiding this comment.
nit: rename to getAllApiDirectories as it is easy to understand? And add a doc for the function
| : item; | ||
|
|
||
| if (fs.lstatSync(itemPath).isDirectory()) { | ||
| if (fs.existsSync(path.join(itemPath, 'exchange.json'))) { |
There was a problem hiding this comment.
nit: Add a comment "Only directories with exchange.json are considered as valid APIs"
| price: 20.99 | ||
| - description: Order received the next business day | ||
| id: '003' | ||
| name: Overn |
There was a problem hiding this comment.
Wait, was this a manual change or did the IPA team add this?
If it's the IPA team, we'll have to ask them to revert this as we probably just want the classname to be ShopperBaskets as its the default right?
There was a problem hiding this comment.
IPA team made this change to set the x-sdk-classname to ShopperBaskets v1.
| } | ||
| return name.replace(/\s+/g, '').replace('OAS', ''); | ||
|
|
||
| return version !== 'v1' ? apiName + version.toUpperCase() : apiName; |
There was a problem hiding this comment.
This seem to be opposite of what you are doing for Shopper Baskets? Why do we need this?
There was a problem hiding this comment.
This was my adjustment to allow for a more generic v2 as per your comment above.
Here, if apiVersion in exchange.json is v1, we don't append V1. This results in just the api name being returned. Ie. ShopperProducts
If the apiVersion in exchange.json is v2, this returns ShopperProductsV2.
ShopperBaskets is a special case where the API team have set x-sdk-classname to ShopperBasketsV1 in the -internal OAS spec. So the API name returned here has to match that (ie. ShopperBasketsV1) since the imports in the top level index.ts are generated using these API names by Handlebars rather than produced by the OAS generator.
We don't want to append V1 to other APIs (ie. we don't want ShopperProductsV1).
This PR contains the following changes to support handling both ShopperBaskets v1 and v2:
With this change, we can now look for the exchange.json files in the following directory structure:
A fix to handle
-internalvs-publicOAS files. The generator will only consider-internalfiles.A template adjustment to handle differences in
ShopperProductQueryParametersbetweengetProductandgetProducts(Thanks @joeluong-sfcc for this!)A test update following the change to allow unknown query parameters.
Update to api versions + a manual fix to
x-sdk-classnameformat.Testing these changes:
Generate the SDk with
npm run renderTemplatesRun
npm run testVerify that all tests pass and that the generated SDK has no errors (areas to look at would be ShopperBaskets V1 / V2, ShopperProducts, ShopperContexts, and the top level
index.ts)