Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Adds nodejs-axios codegen support #236

Closed
wants to merge 3 commits into from
Closed

[WIP] Adds nodejs-axios codegen support #236

wants to merge 3 commits into from

Conversation

someshkoli
Copy link
Contributor

Closes #135
Adds support for axios codegen as requested in #135

someshkoli and others added 3 commits March 24, 2020 03:24
@someshkoli
Update README.md
92c2430 
Example snippet had improper declaration of convert function and request instance.
Fixed and tested
@someshkoli
Merge remote-tracking branch 'upstream/develop' into develop
53a3a44 
Merging from remote develop branch
@someshkoli
Adds support for axios codegen
d915c75
@someshkoli someshkoli changed the title Codegen/axios [WIP] Adds nodejs-axios support Mar 30, 2020
@someshkoli someshkoli changed the title [WIP] Adds nodejs-axios support [WIP] Adds nodejs-axios codegen support Mar 30, 2020
@fjufju
Copy link

fjufju commented Apr 30, 2020

what about #232?
Is this somehow similar?

@someshkoli
Copy link
Contributor Author

someshkoli commented Apr 30, 2020 •
edited
Loading

what about #232?
Is this somehow similar?

I guess. I haven't tested out his version yet. Also didn't know about this pr before I made mine :/ .

@fjufju
Copy link

fjufju commented Apr 30, 2020

There is also #226.
@someshkoli do you know who from members can review this?
I really want axios code generator

@someshkoli
Copy link
Contributor Author

There is also #226.
@someshkoli do you know who from members can review this?
I really want axios code generator

I'm not sure

@fjufju
Copy link

fjufju commented May 4, 2020

@ajwad-shaikh could you please take a look at this.
Axios support would be awesome, especially for React developers

@ajwad-shaikh
Copy link
Contributor

@fjufju I would love to take a look, but I don't think it'll make a difference. The PR requires an approving review from one of the maintainers and I'm just another contributor.

My own PR #238 is awaiting review for about a month now. The maintainers are having a tough time dealing with the shift to remote working practices.

I suppose @shreys7 will be happy to take a look at this as soon as he finds time.

@someshkoli
Copy link
Contributor Author

someshkoli commented May 4, 2020 •
edited
Loading

Yeah the maintainers will look into it when they have time. As a work around, y'all (@fjufju @chriscn) can use a tool that I built checkout. It doesn't have axios support yet bt if you really want it I'll be happy to add it. it'll barely an hour for me to add axios to it.

@shreys7
Copy link
Member

shreys7 commented May 5, 2020

Hey @someshkoli. below is an example of Nodejs-Request snippet generation

var request = require('request');
var options = {
  'method': 'POST',
  'url': 'https://postman-echo.com/post',
  'headers': {
    'Content-Type': ['text/html', 'text/plain']
  },
  body: "<html>\n  Test Test !@#$%^&*()+POL:},'';,[;[;\n</html>"

};
request(options, function (error, response) {
  if (error) throw new Error(error);
  console.log(response.body);
});

Similarly, if you notice Nodejs-Native snippets, we follow the same construct, i.e. define a option/configuration object before and then pass it to the actual library sdk function. Thus, we would like to use that syntax for axios too. e.g.

const axios = require('axios');
const data = "sample data";

const config = {
  'method': 'post',
  'url': 'https://postman-echo.com/post',
  'headers': {        
   'Content-Type': 'text/html, text/plain'
  },
  data : data
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

I will review it once again once you change the code generation to follow the above construct. I have some other comments too, will let you know once these changes are done. 🙂

@someshkoli
Copy link
Contributor Author

Hey @someshkoli. below is an example of Nodejs-Request snippet generation

var request = require('request');
var options = {
  'method': 'POST',
  'url': 'https://postman-echo.com/post',
  'headers': {
    'Content-Type': ['text/html', 'text/plain']
  },
  body: "<html>\n  Test Test !@#$%^&*()+POL:},'';,[;[;\n</html>"

};
request(options, function (error, response) {
  if (error) throw new Error(error);
  console.log(response.body);
});

Similarly, if you notice Nodejs-Native snippets, we follow the same construct, i.e. define a option/configuration object before and then pass it to the actual library sdk function. Thus, we would like to use that syntax for axios too. e.g.

const axios = require('axios');
const data = "sample data";

const config = {
  'method': 'post',
  'url': 'https://postman-echo.com/post',
  'headers': {        
   'Content-Type': 'text/html, text/plain'
  },
  data : data
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

I will review it once again once you change the code generation to follow the above construct. I have some other comments too, will let you know once these changes are done. 🙂

Sure I'll add these changes

@someshkoli
Copy link
Contributor Author

I have made the above changes and made a new PR as this one has some additional unwanted changes. #245

@someshkoli someshkoli closed this May 5, 2020
@someshkoli someshkoli mentioned this pull request May 6, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriscn chriscn chriscn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can you please add the support for axios
5 participants
@someshkoli @fjufju @ajwad-shaikh @shreys7 @chriscn