Babel setup for async/await #1692

hameleonka · 2017-12-07T03:37:59Z

@rpl this PR adds babel support for async/await with transpilation to ES2015 generators for backward compatibility with node 6 + working example of function converted to async/await syntax in linter.js. Please check it out.

…ators for backward compatibility with node 6 + working example of function converted to async/await syntax in linter.js

rpl

This first set of changes looks good (besides some super small nit, the main fix needed is just the one related to the package.json changes).

None of the opened pull request are currently applying changes to the src/linter.js module, and it should be a module that does not change too much in the pull requests that add or fix the validation rules, and so it looks a good module to start this conversion from, good choice.

There are more functions in this file that will be converted, I guess that your goal was to get a preliminary feedback on the first step and then keep working on it, and so I'm not approving this pull request yet (but you got my approval on this first step ;-))

rpl · 2017-12-07T12:15:30Z

.eslintrc

@@ -2,7 +2,7 @@
  "rules":{
    "no-console": 0,
    // Inherited from airbnb via eslint-config-amo
-    'function-paren-newline': 0,
+    "function-paren-newline": 0


rpl · 2017-12-07T12:19:00Z

package.json

@@ -35,15 +35,20 @@
    "babel-eslint": "8.0.2",
    "babel-jest": "21.2.0",
    "babel-loader": "7.1.2",
+    "babel-plugin-syntax-async-functions": "^6.13.0",


Every dependency is tha package.json file should be an exact version (without the ^), so that the version is fixed and greenkeeper can open a pull request for us when one of our dependency released a new version on npm.

rpl · 2017-12-07T12:33:35Z

src/linter.js

+        throw err;
+      }
+    }
+    throw invalidMessage;


👍 it looks great (I would add an empty line at line 289, but that is just a small nit)

rpl · 2017-12-07T12:40:09Z

tests/test.linter.js

@@ -246,15 +246,15 @@ describe('Linter', () => {
    addonLinter.io = { files: { whatever: {} } };
    addonLinter.io.getFile = () => Promise.resolve();
    addonLinter.getScanner = sinon.stub();
-    class fakeScanner {
+    class FakeScanner {


Thanks! (it seems a convention that it could be also nice to enforce on all our sources using an eslint rules, but we can evaluate that in a follow up because I guess that an eslintrc rule could catch more of these and they would be changes unrelated to this pull request goal).

…nctions

hameleonka · 2017-12-09T04:41:46Z

@rpl I finished rewriting linter.js. Could you please check it out when you have a chance.

rpl

Hi Natalia,
this PR looks pretty good, the new async functions are so much more readable of their previous version, thanks you a lot.

follows the new round of review comments (related to a couple of very small tweaks and some optional small nits).

rpl · 2017-12-10T18:49:38Z

package.json

    "jed": "1.1.1",
-    "pino": "^4.6.0",
+    "pino": "4.6.0",


@EnTeQuAk it seems that in this file there were some pre-existent dependency without a strict version, it could be related to yarnpkg/yarn#1088.

rpl · 2017-12-10T19:32:22Z

src/linter.js

-            scannedFiles: [filename],
-          });
-        }
+    // First: check that this file is under our 2MB parsing limit. Otherwise


I've been following the code that make sure that this.io.files map contains the expected file entries, and I've also been discussing about it a bit with @EnTeQuAk today, and it seems that the this.io.files is going to be there mainly because scanFile is not usually called directly, but it is called internally by the Linter class when we call scan (also confirmed by looking at the related test cases).

Nevertheless, it seems that calling this.io.getFile also ensure that the filename is going to be in the this.io.files map of file entries, and so it could be safer for now to still have a const fileData = await this.io.getFile(...) at line 320, so that we reduce chances of introducing a regression during the async/await rewrite (and look into make this "implicit dependency" more explicit in a follow up).

rpl · 2017-12-13T16:44:05Z

src/linter.js

+      };
+    } else {
+      const fileData = await this.io.getFile(filename, ScannerClass.fileResultType);
+      if (ScannerClass !== BinaryScanner && ScannerClass !== FilenameScanner) {


once we move line 339 to line 320 (near to where it used to be called in the previous version) we can merge this if with the else at line 338.

@rpl I moved fileData from line 339 to line 320, but I don't understand how can I merge else on line 338 with if from line 340. It seems for me like they are both necessary.

@hameleonka yeah, don't worry about that, you are right.

Also I think that it is better that we don't change too much this function in this PR, while we are rewriting many of these functions into async functions (and it sound that making this function a bit more readable could be part of the issue mentioned at line 354).

rpl · 2017-12-13T16:53:52Z

src/linter.js

+    io.setScanFileCallback(this.shouldScanFile);
+    this.io = io;
+
+    const addonMetadata = await this.markSpecialFiles(await this.getAddonMetadata());


I'm wondering if we could make this two await more visible and readable, eg.:

let addonMetadata = await this.getAddonMetadata(); addonMetadata = await this.markSpecialFiles(addonMetadata);

or at least move then into two different lines:

const addonMetadata = await this.markSpecialFiles( await this.getAddonMetadata() );

(I kind of prefer the first one which make it also a bit more visible which is the order of the two awaits).

rpl · 2017-12-13T16:59:35Z

src/linter.js

+    try {
+      await this.extractMetadata(deps);
+      const files = await this.io.getFiles();
+      if (this.config.scanFile) {


Nit, let's add one empty line here (just to aid readability)

rpl · 2017-12-13T17:08:36Z

src/linter.js

+      this.print(deps._console);
+      // This is skipped in the code coverage because the
+      // test runs against un-instrumented code.
+      /* istanbul ignore if  */


Nit, do you mind to remove one space from the end of this comment? (it was already there, but given that we are touching this function... ;-), same at line 492)

rpl · 2017-12-13T17:15:36Z

src/linter.js

    }
-    return this.scan(deps).then(() => this.output);
+    await this.scan(deps);


Nit, do you mind to add one empty line at line 504 (only to make it a bit more visible).

rpl

@hameleonka this PR seems to be almost ready, this is likely to be the last round of review comments.

The most important fix that I think we need in this PR is the ones related to Object.prototype.hasOwnProperty.call vs. ... in ....

rpl · 2017-12-15T14:25:30Z

src/linter.js

+    await checkMinNodeVersion();
+
+    const stats = await this.checkFileExists(this.packagePath);
+    let io;


Nit, it seems that we could just set this.io at lines 390, 393 and 398 and change line 401 to call this.io.setScanFileCallback(...) and remove the let io; here.

rpl · 2017-12-15T14:44:26Z

src/linter.js

+      await this.extractMetadata(deps);
+      const files = await this.io.getFiles();
+
+      if (this.config.scanFile) {


Nit, It seems that we can merge this two nested if into a single:

if (this.config.scanFile && !this.config.scanFile.some(...)) { ... }

rpl · 2017-12-15T14:50:21Z

src/linter.js

+
+      // Known libraries do not need to be scanned
+      const filesWithoutJSLibraries = Object.keys(files).filter((file) => {
+        return !(file in this.addonMetadata.jsLibs);


The previous version of this line was using Object.prototype.hasOwnProperty.call instead of in, I think that we should still use Object.prototype.hasOwnProperty here to avoid new regression related to the subtle differences between in and Object.prototype.hasOwnProperty.

rpl · 2017-12-15T14:52:46Z

src/linter.js

-        return this.addonMetadata;
-      });
+    const files = await this.io.getFiles();
+    if (constants.MANIFEST_JSON in files) {


The previous version of this line was using Object.prototype.hasOwnProperty.call instead of in, I think that we should still use Object.prototype.hasOwnProperty here to avoid a new regression related to the subtle differences between in and Object.prototype.hasOwnProperty.

hameleonka · 2017-12-16T03:43:04Z

@rpl I've made all changes that you've mentioned above. Please check it again.

rpl

@hameleonka it looks great, thanks! 🎉

I added a comment related to a very small nit related to the indentation of a single line, eslint doesn't (currently) complain and it is not a big deal.

@EnTeQuAk This should be now ready to land and officially start the conversion of the addons-linter sources \o/
(also, I've built addons-linter from this PR and executed it locally on a couple of extension examples and real world extensions, I have been able to lint them successfully and I didn't notice any regression).

rpl · 2017-12-18T15:27:15Z

src/linter.js

+      const files = await this.io.getFiles();
+
+      if (this.config.scanFile &&
+        !this.config.scanFile.some((f) => Object.keys(files).includes(f))) {


Nit, align !this.config.scanFile... with this.config.scanFile from the previous line.

EnTeQuAk

Personal "removed more lines than added"-award, let's get this landed!

adds babel support for async/await with transpilation to ES2015 gener…

1fdff6c

…ators for backward compatibility with node 6 + working example of function converted to async/await syntax in linter.js

rpl added the state:in_progress label Dec 7, 2017

rpl reviewed Dec 7, 2017

View reviewed changes

Natalia Milovanova added 3 commits December 7, 2017 22:54

rewrites run, scan, scanFile, getAddonMetadata and extractMetadata fu…

e5d6ed6

…nctions

refactors the rest of the promises in linter.js

3f0472d

Merges tag 0.30.0 into branch

586d574

EnTeQuAk mentioned this pull request Dec 13, 2017

Async/await refactoring for files in io folder except directory.js #1697

Merged

rpl requested changes Dec 13, 2017

View reviewed changes

fixes comments from second of review

ac1237d

rpl requested changes Dec 15, 2017

View reviewed changes

reverts unnecessary chahges from third round of review

67a6c41

rpl previously approved these changes Dec 18, 2017

View reviewed changes

EnTeQuAk dismissed rpl’s stale review via cb10b15 December 19, 2017 15:23

EnTeQuAk added 2 commits December 19, 2017 16:23

Merge branch 'master' into 1536-babel-setup

cb10b15

Merge branch 'master' into 1536-babel-setup

cf33478

EnTeQuAk approved these changes Dec 19, 2017

View reviewed changes

EnTeQuAk added this to the 2018.01.11 milestone Dec 19, 2017

EnTeQuAk merged commit 2025658 into mozilla:master Dec 19, 2017

rpl removed the state:in_progress label Dec 20, 2017

Babel setup for async/await #1692

Babel setup for async/await #1692

Uh oh!

Conversation

hameleonka commented Dec 7, 2017

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hameleonka commented Dec 9, 2017

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpl Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hameleonka commented Dec 16, 2017

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EnTeQuAk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpl Dec 15, 2017 •

edited

Loading