Added ES6 Snippet support for NodeJS-Request by sastava007 · Pull Request #164 · postmanlabs/postman-code-generators

sastava007 · 2020-02-22T17:28:03Z

What it does
Allows the user to generate the code snippet for NodeJS Request in ES6 format.
This is in response to issue #115 requested by one of the users.

Changes I made
~~Created a new folder under ./codegens named nodejs-requestES6 which contains all the code for making requests along with newman and unit test files.~~

Registered a new parameter ES6_enabled in options() which allows the user to toggle between whether codegen generate snippet with ES6 features or not.
By default ES6_enabled is set to false
Also added the new created option parameter in structure.test.js file to register it as a valid option id.
Added unit tests to verify the generated snippets.

Follow Up
This PR is first in the series of upcoming PRs which will add the feature of generating code snippet in ES6 notation with NodeJS.

@abhijitkane @shreys7 Please review!

umeshp7 · 2020-02-24T07:11:32Z

Hey! Thanks for the contribution. @sastava007
This actually could be just changes in the existing NodeJS-Request code-gen. You could either change the existing snippet completely to respect ES6 or you could add an option for enabling ES6 in the same codegen.
There is very little difference between this and the existing NodeJS-Request codegen to qualify as a new codegen. We ideally use a new variant when the underlying library is different and the language is same.

sastava007 · 2020-02-24T08:06:41Z

@umeshp7 Thanks for your feedback, I was also thinking of the same changes.
What we can do is that we can pass an extra parameter ES6_enabled : true/false with by default value as false in options while calling convert() and based on that we can return the respective snippet.
What do you say?

shreys7 · 2020-02-24T09:28:38Z

@sastava007 yes, this would be similar to how other options are used while generating snippet. Add it to the options array defined in getOptions() function, and then use the option everywhere you need to generate ES6 enabled snippet. Add the support via options now, the default value can be decided later.

…cture.test.js file to respect the new changes

sastava007 · 2020-02-24T12:18:08Z

@umeshp7 @shreys7
I have made the changes as you mentioned and also registered the new option parameter in structure.test.js to avoid any error while testing.
I have tested the above changes locally by passing different values to ES6_enabled parameter to check for any leak and it worked all the time.

Please review!

umeshp7 · 2020-02-24T12:28:46Z

@sastava007 Okay, we'll review. Could you also update the description of this PR to describe the changes you have done now?

sastava007 · 2020-02-24T14:12:30Z

@umeshp7 Updated!

shreys7 · 2020-02-25T08:10:02Z

@sastava007 need unit tests around this to verify the generated snippet with option set to true/false

sastava007 · 2020-02-25T13:28:47Z

@shreys7 Have added, please review.

shreys7

Do we really need a new variable called ES6_snippet? Can't we keep just one snippet variable and make some required changes according to the ES6_enabled option's value, as I see a lot of code being repeated for generating snippet and ES6_snippet.

shreys7 · 2020-02-25T18:12:58Z

codegens/nodejs-request/lib/request.js

    optionsArray.push(indentString + 'followRedirect: false');
  }
+  if (options.ES6_enabled === true) {
+    optionsArray.push(indentString + 'ES6_enabled: true');


Why do we need this on line 74? @sastava007

@shreys7
I thought that it would be better to have a separate snippet for ES6 instead of using bunch of if statements. But, no problem I'll make it with single snippet only.

And for this,

if (options.ES6_enabled === true) { optionsArray.push(indentString + 'ES6_enabled: true');

I have mistaken this and thought that we need to pass it to options property, but just I realized that it's only required till the snippet is generated and after that, it's of no use.

I'll fix all this, and made a separate PR by deleting this one.

@sastava007 Hmm, I see. See if you can reduce the number of if statements. If that's causing a lot of if-else statements, then we can go with your approach.

shreys7 · 2020-02-25T18:13:38Z

codegens/nodejs-request/test/unit/snippet.test.js

+        }
+
+        expect(snippet).to.be.a('string');
+        expect(snippet).to.include('ES6_enabled: true');


again, why does this need to be included in the generated snippet? @sastava007

shreys7 · 2020-02-25T18:13:52Z

codegens/nodejs-request/test/unit/snippet.test.js

+        }
+
+        expect(snippet).to.be.a('string');
+        expect(snippet).to.not.include('ES6_enabled: true');


same comment as above

shreys7 · 2020-02-25T18:18:05Z

codegens/nodejs-request/lib/request.js

+    ES6_snippet += 'const fs = require(\'fs\');\n';
  }
  snippet += 'var options = {\n';
+  ES6_snippet += 'const options = {\n';


This should be var or more preferably let instead of const options

umeshp7 · 2020-02-27T11:49:04Z

The changes requested should have been updated on this PR itself for the purposes of keeping proper track of the progress.

sastava007 · 2020-02-27T11:59:47Z

Actually, I pushed those commits directly to the default develop branch itself. Because of which whenever I create any new branch for resolving other features/issue all the previous commits were also showing in it.

So I thought that it would be better if I delete that PR and start again with a dedicated branch to a particular issue this would help me to keep working on the different issues until a previous one is under review.

Added ES6 Snippet support for NodeJS-Request

7146c84

sastava007 requested review from abhijitkane and shreys7 February 22, 2020 17:28

Added ES6 support to NodeJS-Request and maded similar changes in stru…

45e127d

…cture.test.js file to respect the new changes

Added unit tests for ES6 feature support

6b5314a

sastava007 removed the request for review from abhijitkane February 25, 2020 13:29

shreys7 requested changes Feb 25, 2020

View reviewed changes

sastava007 closed this Feb 27, 2020

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade eslint-plugin-lodash from 2.4.5 to 2.7.0 #212

Closed

snyk-bot mentioned this pull request May 31, 2020

[Snyk] Upgrade eslint-plugin-lodash from 2.4.5 to 2.7.0 #272

Closed

snyk-bot mentioned this pull request Jun 30, 2020

[Snyk] Upgrade eslint-plugin-lodash from 2.4.5 to 2.7.0 #296

Merged

snyk-bot mentioned this pull request Oct 10, 2020

[Snyk] Upgrade eslint-plugin-lodash from 2.4.5 to 2.7.0 #360

Closed

Conversation

sastava007 commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umeshp7 commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sastava007 commented Feb 24, 2020

Uh oh!

shreys7 commented Feb 24, 2020

Uh oh!

sastava007 commented Feb 24, 2020

Uh oh!

umeshp7 commented Feb 24, 2020

Uh oh!

sastava007 commented Feb 24, 2020

Uh oh!

shreys7 commented Feb 25, 2020

Uh oh!

sastava007 commented Feb 25, 2020

Uh oh!

shreys7 left a comment

Choose a reason for hiding this comment

Uh oh!

shreys7 Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

sastava007 Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

shreys7 Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

shreys7 Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

shreys7 Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

shreys7 Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

umeshp7 commented Feb 27, 2020

Uh oh!

sastava007 commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sastava007 commented Feb 22, 2020 •

edited

Loading

umeshp7 commented Feb 24, 2020 •

edited

Loading