Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

feat: adding support to oas-to-snippet for alternative language targets #1261

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

erunion
Copy link
Member

@erunion erunion commented Apr 30, 2021

☁️   CI App

🧰 What's being changed?

This updates @readme/oas-to-snippet to add support alternative language targets!

Previously if you wanted to generate a Node snippet, you'd do:

generateCodeSnippet(oas, operation, formData, authData, 'node', oasUrl);

This would generate you a node-fetch snippet and that's great but HTTPSnippet offers all sorts of other Node targets like axios or request that you couldn't use — with this change you can now do the following:

generateCodeSnippet(oas, operation, formData, authData, ['node', 'axios'], oasUrl);

With the language argument as an array we'll now do a lookup in our supported targets for node and if axios is there we'll generate an Axios snippet!

🧬 Testing

I've mostly rewritten supportedLanguages and the core language determination in this library along with a ton of new tests for this and all of the new targets we're now exposing, but with that there's a few points:

@erunion erunion added the type:enhancement A potential new feature to be added, or an improvement we could make label Apr 30, 2021
@erunion erunion requested a review from domharrington April 30, 2021 00:04
@erunion erunion temporarily deployed to explorer-pr-1261 April 30, 2021 00:04 Inactive
@erunion erunion requested review from Dashron and ilias-t April 30, 2021 00:04
Comment on lines -30 to -49
const harOverride = {
log: {
entries: [
{
request: {
method: 'GET',
url: 'https://dash.readme.io/api/v1/categories/',
httpVersion: 'HTTPS/1.1',
headers: [
{
name: 'authorization',
value: 'Basic xxx',
},
],
queryString: [],
},
},
],
},
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this hardcoded HAR in favor of using one that I copied from HTTPSnippet into https://github.com/readmeio/har-examples (we use this repo for testing api snippets).

};

const codeSnippet = generateCodeSnippet(oas, operation, {}, {}, 'node', oasUrl, harOverride);
const codeSnippet = generateCodeSnippet(null, null, null, null, 'node', null, harExamples.full);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made all of these arguments null because the only ones that should be getting picked up are the language and override.

Comment on lines +282 to +290
if ('opts' in supportedLanguages[lang].httpsnippet.targets[target]) {
// eslint-disable-next-line jest/no-conditional-expect
expect(supportedLanguages[lang].httpsnippet.targets[target].opts).toStrictEqual(expect.any(Object));
}

if ('install' in supportedLanguages[lang].httpsnippet.targets[target]) {
// eslint-disable-next-line jest/no-conditional-expect
expect(supportedLanguages[lang].httpsnippet.targets[target].install).toStrictEqual(expect.any(String));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this conditional testing, but Jest doesn't offer any "assert this value as something if it exists, if it doesn't its ok". I could do an assertion on this with JSON Schema but that'd require loading in ajv or some other validator. No thanks!

@@ -1,80 +1,212 @@
const supportedLanguages = {
module.exports = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note that by putting all these alternative targets into these configs that we're going to be exposing them in the APIRequest module with some work I'm doing to support custom code samples for manual APIs. Shouldn't be a problem, but FYI regardless.

Copy link
Contributor

@Dashron Dashron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing jumped out at me.

@erunion erunion merged commit ea8d42a into next Apr 30, 2021
@erunion erunion deleted the feat/support-alternate-language-targets branch April 30, 2021 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type:enhancement A potential new feature to be added, or an improvement we could make
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants