Uses ESLint to catch errors and fixes ESLint errors #173 #180

ghusse · 2017-12-09T23:05:00Z

Adds ESLint to the build process:

Adds .eslintrc.json files to configure ESLint at different levels
Fixes some bugs / warnings revealed by ESLint
Adds inline comments for disabling ESLint in some situations
Launches ESLint in Travis CI
Launches ESLint in the precommit hook

For some files in integration tests, I had to disable ESLint because the syntax is a mix of the import export from ES6, and the support of a special function named import. I suppose that this is revealing an issue with the current syntax supported by parcel, but in the meantime, I just ignored these files as they cannot be parsed by ESLint.

For the moment, only the recommended rules from ESLint have been applied. Feel free to propose customizations.

Closes #173

ghusse

I added inline comments to explain modifications in this PR or ask questions about the current code.

ghusse · 2017-12-09T23:08:17Z

src/Bundler.js

@@ -262,19 +261,21 @@ class Bundler extends EventEmitter {
    try {
      return await this.resolveAsset(dep.name, asset.name);
    } catch (err) {
-      if (err.message.indexOf(`Cannot find module '${dep.name}'`) === 0) {
-        err.message = `Cannot resolve dependency '${dep.name}'`;
+      let thrown = err;


Fixes no-ex-assign

ghusse · 2017-12-09T23:09:49Z

src/transforms/uglify.js

@@ -1,7 +1,5 @@
 const {AST_Node, minify} = require('uglify-js');
 const {toEstree} = require('babel-to-estree');
-const types = require('babel-types');


Are these require statements required for babel? I removed them because the variables are not used.

ghusse · 2017-12-09T23:10:17Z

src/worker.js


 let parser;

-function emit(event, ...args) {


This function does not seem to be called

ghusse · 2017-12-09T23:10:55Z

src/Bundler.js

@@ -2,7 +2,6 @@ const fs = require('./utils/fs');
 const Resolver = require('./Resolver');
 const Parser = require('./Parser');
 const WorkerFarm = require('./WorkerFarm');
-const worker = require('./utils/promisify')(require('./worker.js'));


Is it normal that worker was not used?

yeskunall

Wow, that looks like some work you put in there @ghusse 😁 thanks for getting this in so quickly. I've made some comments based on what I've noticed. Might not be right in all the places, but these are just my 2 cents.

yeskunall · 2017-12-10T00:07:53Z

src/Asset.js

@@ -73,7 +72,7 @@ class Asset {

    let resolved = path
      .resolve(path.dirname(from), url)
-      .replace(/[\?#].*$/, '');
+      .replace(/[?#].*$/, '');


I'm not very good with regex, so simply ignore if I'm being stupid here, but looks like ESLint removed \, which could potentially change the result of resolved. Wouldn't that mean that it would now also remove ? and # from the path? 🤔

In this case, they are the same.

[\?#] => match any item in the list: - literal ? - literal # [?#] => match any item in the list: `` ?#

Yeah @brandon93s I'm the type that has to look up SO for the simplest regex's. 😆 but thanks for looking into it!

Yep: ESLint was complaining about unecessary escapes.

yeskunall · 2017-12-10T00:10:09Z

src/FSCache.js

@@ -70,7 +71,9 @@ class FSCache {
    try {
      await fs.unlink(this.getCacheFile(filename));
      this.invalidated.delete(filename);
-    } catch (err) {}
+    } catch (err) {


Did ESLint complain about this?

Also, never really a good idea to have anything fail silently, imo. But that's another discussion.

Yes, that's why I added a comment: I did not want to deactivate this rule

yeskunall · 2017-12-10T00:12:04Z

src/assets/CSSAsset.js

 const postcssTransform = require('../transforms/postcss');
 const CssSyntaxError = require('postcss/lib/css-syntax-error');

-const URL_RE = /url\s*\(\"?(?![a-z]+:)/;
+const URL_RE = /url\s*\("?(?![a-z]+:)/;


Here too. ESLint seems to have removed some bit of regex. Again, ignore if I'm wrong. 😅

Same here. The \ was an unnecessary character escape. Same functionality.

yeskunall · 2017-12-10T00:13:01Z

src/builtins/bundle-url.js

@@ -22,7 +22,7 @@ function getBundleURL() {
 }

 function getBaseURL(url) {
-  return ('' + url).replace(/^((?:https?|file|ftp):\/\/.+)\/[^\/]+$/, '$1') + '/';
+  return ('' + url).replace(/^((?:https?|file|ftp):\/\/.+)\/[^/]+$/, '$1') + '/';


Here too. ESLint seems to have removed some bit of regex. Again, ignore if I'm wrong. 😅

Same here. The \ was an unnecessary character escape. Same functionality.

yeskunall · 2017-12-10T00:14:05Z

src/packagers/Packager.js

@@ -17,6 +17,7 @@ class Packager {

  async start() {}

+  // eslint-disable-next-line no-unused-vars


Why is this particular one needed? I don't see any unused vars.

asset is unused. Guessing it was left to be self-documenting since this requires implementation by extending classes.

Exactly: I prefered not to remove it, because it seems to be useful for documentation

yeskunall · 2017-12-10T00:14:16Z

src/utils/is-url.js

@@ -4,7 +4,7 @@ const isURL = require('is-url');
 const ANCHOR_REGEXP = /^#/;

 // Matches scheme (ie: tel:, mailto:, data:)
-const SCHEME_REGEXP = /^[a-z]*\:/i;
+const SCHEME_REGEXP = /^[a-z]*:/i;


Here too. ESLint seems to have removed some bit of regex. Again, ignore if I'm wrong. 😅

Same here. The \ was an unnecessary character escape. Same functionality.

yeskunall · 2017-12-10T00:19:44Z

.eslintrc.json

+{
+  "extends": "eslint:recommended",
+  "parserOptions": {
+    "ecmaVersion": 8    


Setting this to 8 will result in some features not supported by Node 6 to no longer be caught by ESLint, so we should beware of this.

Actually, babel seem used everywhere in parcel. It allows to use async/await for instance, which is not supported by Node 6 and si part of ecmaScript 2017 or 8. That's why this version here is needed.

Yup, you're right. The whole codebase is later transpiled using Babel. This should be okay. Thanks for the clarification 😅

yeskunall · 2017-12-10T00:23:12Z

@ghusse noticed a trend where ESLint just removed some parts of the regex. I've mentioned that in quite a few places in my review (sorry about that), but other than that, this looks really good. Appreciate it. 💖

itsMapleLeaf · 2017-12-10T01:18:29Z

I believe those are in places where the escape is unnecessary, so it should be fine 🙂

yeskunall · 2017-12-10T01:21:26Z

Yup, that's what ESLint says. But I pushed some code once that removed those escapes without checking and my tests started failing. Admittedly, I didn't know what I was doing with regex then, and I still don't. 😅

Thanks tho @kingdaro 💖

brandon93s · 2017-12-10T06:43:11Z

bin/cli.js

@@ -62,13 +62,15 @@ program
  });

 program.on('--help', function() {
+  /* eslint-disable no-console */


Since this is a CLI, should we just allow console globally in the eslintrc? Several files required this disable.

Ok, I'll change that

yeskunall · 2017-12-10T06:44:42Z

So the general consensus is that I'm just bad at regex. Feel free to ignore all comments I made about the regex issues.

However, @ghusse I did notice a lot of /* eslint-enable no-console */ (or something along the lines) during my initial review, but forgot to mention them. I think that for all purposes, this should be safe to turn off, or at the very list, switched to only warn the user.

EDIT: as @brandon93s points out, it should be safe to turn it off.

brandon93s · 2017-12-10T06:44:55Z

package.json

-    "*.{js,json,md}": ["prettier --write", "git add"]
+    "*.{js,json,md}": [
+      "prettier --write",
+      "eslint --fix",


Running eslint on md files?

Yeah, it should instead do an npm run lint before lint-staged in the precommit hook.

woops. Thanks. I'll change that

ghusse · 2017-12-10T22:15:00Z

I pushed new modifications to this PR. Let me know if there are other changes you'd like to see.

brandon93s · 2017-12-11T05:45:06Z

Are all of the sub eslintrc necessary? Can they be consolidated into the parent by promoting the rules? I see they're extending one another, but would want to reduce as much maintenance overhead as possible.

ghusse · 2017-12-11T06:57:36Z

I created a sub-config file everywhere the context changed, or different rules have to be applied. That's the way eslint is handling these cases.

I can comment every file to explain why I created it if you want.

ghusse

I commented every configuration file to explain why I created it

ghusse · 2017-12-11T07:51:06Z

.eslintrc.json

@@ -0,0 +1,10 @@
+{


That is the master configuration file

ghusse · 2017-12-11T07:52:45Z

bin/.eslintrc.json

@@ -0,0 +1,6 @@
+{


I created this one in the bin directory to deactivate the no-console rule just here.

I propose not to deactivate it everywhere, because usage of console.log can be signs of forgotten test code. But if you prefer I can move it to the general configuration (but deactivate it in tests)

It was a great time with you

ghusse · 2017-12-11T07:54:06Z

src/builtins/.eslintrc.json

@@ -0,0 +1,9 @@
+{


I needed to specify the context used by files in builtins, as a browser context. Some files are using features specific to browsers (such as WebSocket for what I remember)

ghusse · 2017-12-11T07:54:49Z

test/.eslintrc.json

@@ -0,0 +1,6 @@
+{


Needed to specify that in this repository, the environment is different (it is using mocha)

ghusse · 2017-12-11T07:55:48Z

test/integration/.eslintrc.json

@@ -0,0 +1,10 @@
+{


Integration test code is using browser-specific features and special functions output and import that are used in different files. This is not the case in the rest of the code.

ghusse · 2017-12-11T07:56:41Z

test/integration/dynamic-esm/.eslintrc.json

@@ -0,0 +1,9 @@
+{


This directory contains code that tests the ES6 syntax for importing and exporting code. ESLint needs a specific configuration to support this syntax, and we cannot use inline-configuration comments for this.

ghusse · 2017-12-11T07:57:05Z

test/integration/es6-default-only/.eslintrc.json

@@ -0,0 +1,6 @@
+{


Needed to support the parsing of ES6 import/export syntax

ghusse · 2017-12-11T07:57:13Z

test/integration/es6/.eslintrc.json

@@ -0,0 +1,6 @@
+{


Needed to support the parsing of ES6 import/export syntax

ghusse · 2017-12-11T07:57:19Z

test/integration/fs-import/.eslintrc.json

@@ -0,0 +1,6 @@
+{


Needed to support the parsing of ES6 import/export syntax

ghusse · 2017-12-11T10:30:27Z

Did I introduce regressions? It seems that this build is pretty new, are the test passing on master?

ghusse · 2017-12-11T12:56:04Z

Ok, I need to investigate why tests are failing. It seems that this PR introduces a regression somewhere, as test are passing on master.

ghusse · 2017-12-11T21:26:24Z

Ok, indeed, I introduced regressions with my fixes on prelude.js. I just pushed a version that both work, and statisfies ESLint rule of not creating a function inside a if statement.

ghusse · 2017-12-11T21:37:04Z

Ok, all tests are passing, and all the warnings from master are fixed in this branch.

brandon93s · 2017-12-12T03:38:28Z

src/builtins/.eslintrc.json

@@ -0,0 +1,9 @@
+{


Please add the following:

"parserOptions": { "ecmaVersion": 5 }

This will protect against future regressions (see #169, #227) since these aren't currently processed by Babel.

Ok, I'll do it :-)

Done, and I also replaced one const and one let by var in src/builtins/index.js.

ghusse · 2017-12-12T23:07:35Z

Anything else to change on this PR?

yeskunall · 2017-12-12T23:39:06Z

At this point, you should ping one of the members @ghusse 😁

davidnagli · 2017-12-12T23:45:49Z

@yeskunall No need to ping us, we’re always watching 👀

yeskunall · 2017-12-12T23:48:59Z

@davidnagli

davidnagli · 2017-12-12T23:51:53Z

CONTRIBUTING.md

Did I miss something, or are these exactly the same?

EOL/EOF mismatch between OSes because of git? Sure looks like it.

Sh******

Thanks, I'll change that

I checked the file, it seems that this file is now encoded using LF. I suppose that it was not the case on master. Do you want me to revert changes on that file?

I don’t think it makes any difference

ghusse · 2017-12-13T20:46:28Z

@davidnagli @yeskunall Anything I can do for this PR to be merged?

yeskunall · 2017-12-13T21:07:17Z

I don't have push access @ghusse 😟 (at this point you should really ping someone who has the access)

ghusse · 2017-12-13T22:53:30Z

ping @brandon93s ?

brandon93s · 2017-12-14T03:42:33Z

Looks ready to go to me.

devongovett · 2017-12-20T07:17:03Z

Looks good. can you fix the conflicts?

ghusse · 2017-12-20T21:45:50Z

Conflicts solved (I had to fix other errors introduced by recent commits)

yeskunall · 2017-12-20T22:11:39Z

Hey @ghusse this is killer! Thanks for not giving up on this PR. Do you also mind squashing your commits? It's just a suggestion. 😄

devongovett · 2017-12-21T05:29:48Z

Awesome, thanks for doing this!

No need to squash btw, I'll do that on merge.

* Uses ESLint to catch errors and fixes ESLint errors #173 * Changes ESLint configuration to allows usage of console * Launches npm run lint before git commit * Adds missing ESLint config files * Runs ESLint in AppVeyor * Fixes a regression introduced previously, fixes new warnings * Fixes new ESLint errors introduced in master * Specifies ecmaVersion=5 in builtins and fixes errors in index.js

Uses ESLint to catch errors and fixes ESLint errors parcel-bundler#173

96469cb

ghusse commented Dec 9, 2017

View reviewed changes

yeskunall reviewed Dec 10, 2017

View reviewed changes

brandon93s reviewed Dec 10, 2017

View reviewed changes

ghusse added 3 commits December 10, 2017 15:37

Changes ESLint configuration to allows usage of console

c850a1e

Launches npm run lint before git commit

e103804

Adds missing ESLint config files

c574900

ghusse added 2 commits December 11, 2017 08:42

Merge branch 'master' into master

fe571bf

Merge branch 'master' into master

8b492f2

ghusse commented Dec 11, 2017

View reviewed changes

ghusse added 3 commits December 11, 2017 11:17

Merge branch 'master' into master

d2738bd

Merge branch 'master' into master

d919fa9

Runs ESLint in AppVeyor

526afc8

ghusse added 2 commits December 11, 2017 13:32

Merge branch 'master' into master

26aabe0

Merge branch 'master' into master

dd55256

Fixes a regression introduced previously, fixes new warnings

944f360

ghusse added 2 commits December 11, 2017 22:31

Merge branch 'master' of https://github.com/parcel-bundler/parcel

ec0e1c6

Fixes new ESLint errors introduced in master

9c1215c

brandon93s suggested changes Dec 12, 2017

View reviewed changes

ghusse added 2 commits December 12, 2017 10:47

Merge branch 'master' of https://github.com/parcel-bundler/parcel

437b385

Specifies ecmaVersion=5 in builtins and fixes errors in index.js

2e81db0

davidnagli reviewed Dec 12, 2017

View reviewed changes

brandon93s approved these changes Dec 14, 2017

View reviewed changes

brandon93s requested a review from devongovett December 14, 2017 03:42

Merge branch 'master' of https://github.com/parcel-bundler/parcel

7a0ad22

Merge branch 'master' of https://github.com/parcel-bundler/parcel

7b1714e

devongovett approved these changes Dec 21, 2017

View reviewed changes

devongovett merged commit 434b86c into parcel-bundler:master Dec 21, 2017

		@@ -17,6 +17,7 @@ class Packager {

		async start() {}

		// eslint-disable-next-line no-unused-vars

Uses ESLint to catch errors and fixes ESLint errors #173 #180

Uses ESLint to catch errors and fixes ESLint errors #173 #180

Conversation

ghusse commented Dec 9, 2017 • edited by brandon93s Loading

ghusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeskunall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeskunall Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandon93s Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandon93s Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeskunall commented Dec 10, 2017

itsMapleLeaf commented Dec 10, 2017

yeskunall commented Dec 10, 2017

brandon93s Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeskunall commented Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

yeskunall Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghusse commented Dec 10, 2017

brandon93s commented Dec 11, 2017

ghusse commented Dec 11, 2017

ghusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghusse commented Dec 11, 2017

ghusse commented Dec 11, 2017

ghusse commented Dec 11, 2017

ghusse commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghusse commented Dec 12, 2017

yeskunall commented Dec 12, 2017

davidnagli commented Dec 12, 2017

yeskunall commented Dec 12, 2017

Choose a reason for hiding this comment

yeskunall Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghusse commented Dec 13, 2017

yeskunall commented Dec 13, 2017

ghusse commented Dec 13, 2017

brandon93s commented Dec 14, 2017

ghusse commented Dec 9, 2017 •

edited by brandon93s

Loading

yeskunall Dec 10, 2017 •

edited

Loading

brandon93s Dec 10, 2017 •

edited

Loading

brandon93s Dec 10, 2017 •

edited

Loading

brandon93s Dec 10, 2017 •

edited

Loading

yeskunall commented Dec 10, 2017 •

edited

Loading

yeskunall Dec 10, 2017 •

edited

Loading

yeskunall Dec 12, 2017 •

edited

Loading