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.

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

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

Merged
merged 18 commits into from
Dec 21, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# ESLint does not support both
# - import used as a function, and
# - the export statement
# in the same file

/test/integration/dynamic/index.js
/test/integration/dynamic-css/index.js
/test/integration/dynamic-esm/index.js
/test/integration/dynamic-hoist/index.js
/test/integration/hmr-dynamic/index.js
10 changes: 10 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the master configuration file

"extends": "eslint:recommended",
"parserOptions": {
"ecmaVersion": 8
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

},
"env": {
"node": true,
"es6": true
}
}
4 changes: 3 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@ node_js:
# - '6'
- '8'
cache: yarn
script: yarn test
script:
- yarn test
- yarn lint
sudo: false
2 changes: 2 additions & 0 deletions bin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,15 @@ program
});

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

@brandon93s brandon93s Dec 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change that

console.log('');
console.log(
' Run `' +
chalk.bold('parcel help <command>') +
'` for more information on specific commands'
);
console.log('');
/* eslint-enable no-console */
});

// Make serve the default command
Expand Down
10 changes: 8 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"babel-cli": "^6.26.0",
"babel-preset-env": "^1.6.1",
"cross-env": "^5.1.1",
"eslint": "^4.13.0",
"husky": "^0.14.3",
"less": "^2.7.2",
"lint-staged": "^6.0.0",
Expand All @@ -62,7 +63,8 @@
"format": "prettier --write './{src,bin,test}/**/*.{js,json,md}'",
"build": "babel src -d lib",
"prepublish": "yarn build",
"precommit": "lint-staged"
"precommit": "lint-staged",
"lint": "eslint ."
},
"bin": {
"parcel": "bin/cli.js"
Expand All @@ -71,6 +73,10 @@
"node": ">= 6.0.0"
},
"lint-staged": {
"*.{js,json,md}": ["prettier --write", "git add"]
"*.{js,json,md}": [
"prettier --write",
"eslint --fix",
Copy link
Contributor

Choose a reason for hiding this comment

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

Running eslint on md files?

Copy link
Contributor

@yeskunall yeskunall Dec 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops. Thanks. I'll change that

"git add"
]
}
}
3 changes: 1 addition & 2 deletions src/Asset.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const Parser = require('./Parser');
const path = require('path');
const fs = require('./utils/fs');
const objectHash = require('./utils/objectHash');
Expand Down Expand Up @@ -73,7 +72,7 @@ class Asset {

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

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, they are the same.

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

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

Copy link
Contributor

@yeskunall yeskunall Dec 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep: ESLint was complaining about unecessary escapes.

this.addDependency(
'./' + path.relative(path.dirname(this.name), resolved),
Object.assign({dynamic: true}, opts)
Expand Down
1 change: 0 additions & 1 deletion src/Bundle.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const Path = require('path');
const fs = require('fs');
const crypto = require('crypto');

/**
Expand Down
15 changes: 8 additions & 7 deletions src/Bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it normal that worker was not used?

const Path = require('path');
const Bundle = require('./Bundle');
const {FSWatcher} = require('chokidar');
Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes no-ex-assign


if (thrown.message.indexOf(`Cannot find module '${dep.name}'`) === 0) {
thrown.message = `Cannot resolve dependency '${dep.name}'`;

// Generate a code frame where the dependency was used
if (dep.loc) {
await asset.loadIfNeeded();
err.loc = dep.loc;
err = asset.generateErrorMessage(err);
thrown.loc = dep.loc;
thrown = asset.generateErrorMessage(thrown);
}

err.fileName = asset.name;
thrown.fileName = asset.name;
}
throw err;
throw thrown;
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/FSCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class FSCache {
await fs.writeFile(this.getCacheFile(filename), JSON.stringify(data));
this.invalidated.delete(filename);
} catch (err) {
// eslint-disable-next-line no-console
console.error('Error writing to cache', err);
}
}
Expand Down Expand Up @@ -70,7 +71,9 @@ class FSCache {
try {
await fs.unlink(this.getCacheFile(filename));
this.invalidated.delete(filename);
} catch (err) {}
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did ESLint complain about this?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// Fail silently
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/Logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class Logger {
this.lines += message.split('\n').length;
}

// eslint-disable-next-line no-console
console.log(message);
}

Expand Down
2 changes: 0 additions & 2 deletions src/Server.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
const http = require('http');
const path = require('path');
const url = require('url');
const serveStatic = require('serve-static');

function middleware(bundler) {
Expand Down
4 changes: 1 addition & 3 deletions src/assets/CSSAsset.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
const Asset = require('../Asset');
const postcss = require('postcss');
const valueParser = require('postcss-value-parser');
const path = require('path');
const md5 = require('../utils/md5');
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]+:)/;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const IMPORT_RE = /@import/;
const PROTOCOL_RE = /^[a-z]+:/;

Expand Down
2 changes: 0 additions & 2 deletions src/assets/HTMLAsset.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
const Asset = require('../Asset');
const posthtml = require('posthtml');
const parse = require('posthtml-parser');
const api = require('posthtml/lib/api');
const path = require('path');
const md5 = require('../utils/md5');
const render = require('posthtml-render');
const posthtmlTransform = require('../transforms/posthtml');
const isURL = require('../utils/is-url');
Expand Down
2 changes: 1 addition & 1 deletion src/assets/LESSAsset.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ function urlPlugin(asset) {
return {
install: (less, pluginManager) => {
let visitor = new less.visitors.Visitor({
visitUrl: (node, args) => {
visitUrl: (node) => {
node.value.value = asset.addURLDependency(
node.value.value,
node.currentFileInfo.filename
Expand Down
1 change: 0 additions & 1 deletion src/assets/RawAsset.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const Asset = require('../Asset');
const path = require('path');

class RawAsset extends Asset {
// Don't load raw assets. They will be copied by the RawPackager directly.
Expand Down
9 changes: 9 additions & 0 deletions src/builtins/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the following:

"parserOptions": {
  "ecmaVersion": 5
}

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"extends": "../../.eslintrc.json",
"env": {
"browser": true
},
"rules": {
"no-global-assign": [1]
}
}
4 changes: 2 additions & 2 deletions src/builtins/bundle-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function getBundleURL() {
try {
throw new Error;
} catch (err) {
var matches = ('' + err.stack).match(/(https?|file|ftp):\/\/[^\)\n]+/g);
var matches = ('' + err.stack).match(/(https?|file|ftp):\/\/[^)\n]+/g);
if (matches) {
return getBaseURL(matches[0]);
}
Expand All @@ -22,7 +22,7 @@ function getBundleURL() {
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@brandon93s brandon93s Dec 10, 2017

Choose a reason for hiding this comment

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

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

}

exports.getBundleURL = getBundleURLCached;
Expand Down
2 changes: 1 addition & 1 deletion src/builtins/css-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ function reloadCSS() {

cssTimeout = null;
}, 50);
};
}

module.exports = reloadCSS;
2 changes: 2 additions & 0 deletions src/builtins/hmr-runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ if (!module.bundle.parent) {
}

if (data.type === 'error-resolved') {
// eslint-disable-next-line no-console
console.log('[parcel] ✨ Error resolved');
}

if (data.type === 'error') {
// eslint-disable-next-line no-console
console.error(`[parcel] 🚨 ${data.error.message}\n${data.error.stack}`);
}
};
Expand Down
16 changes: 8 additions & 8 deletions src/builtins/prelude.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,21 @@ require = (function (modules, cache, entry) {
throw err;
}

function localRequire(x) {
return newRequire(localRequire.resolve(x));
}

localRequire.resolve = function (x) {
return modules[name][1][x] || x;
};

var module = cache[name] = new newRequire.Module;
modules[name][0].call(module.exports, localRequire, module, module.exports);
}

return cache[name].exports;
}

function localRequire(x) {
return newRequire(localRequire.resolve(x));
}

localRequire.resolve = function (x) {
return modules[name][1][x] || x;
};

function Module() {
this.bundle = newRequire;
this.exports = {};
Expand Down
1 change: 1 addition & 0 deletions src/packagers/Packager.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class Packager {

async start() {}

// eslint-disable-next-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

async addAsset(asset) {
throw new Error('Must be implemented by subclasses');
}
Expand Down
1 change: 0 additions & 1 deletion src/transforms/babel.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const babel = require('babel-core');
const path = require('path');
const config = require('../utils/config');

module.exports = async function(asset) {
Expand Down
2 changes: 0 additions & 2 deletions src/transforms/uglify.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
const {AST_Node, minify} = require('uglify-js');
const {toEstree} = require('babel-to-estree');
const types = require('babel-types');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const walk = require('babylon-walk');

module.exports = async function(asset) {
await asset.parseIfNeeded();
Expand Down
2 changes: 1 addition & 1 deletion src/utils/is-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@brandon93s brandon93s Dec 10, 2017

Choose a reason for hiding this comment

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

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


module.exports = function(url) {
return isURL(url) || ANCHOR_REGEXP.test(url) || SCHEME_REGEXP.test(url);
Expand Down
1 change: 0 additions & 1 deletion src/visitors/dependencies.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const types = require('babel-types');
const {resolve} = require('path');
const template = require('babel-template');

const requireTemplate = template('require("_bundle_loader")');
Expand Down
1 change: 0 additions & 1 deletion src/visitors/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ module.exports = {
},

CallExpression(path, asset) {
let callee = path.node.callee;
if (referencesImport(path, 'fs', 'readFileSync')) {
let vars = {
__dirname: Path.dirname(asset.name),
Expand Down
1 change: 0 additions & 1 deletion src/visitors/globals.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const template = require('babel-template');
const Path = require('path');
const types = require('babel-types');

Expand Down
14 changes: 5 additions & 9 deletions src/worker.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
require('v8-compile-cache');
const fs = require('./utils/fs');
const Parser = require('./Parser');
const babel = require('./transforms/babel');

let parser;

function emit(event, ...args) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function does not seem to be called

process.send({event, args});
}

exports.init = function(options, callback) {
parser = new Parser(options || {});
callback();
Expand All @@ -25,11 +19,13 @@ exports.run = async function(path, pkg, options, callback) {
hash: asset.hash
});
} catch (err) {
let returned = err;

if (asset) {
err = asset.generateErrorMessage(err);
returned = asset.generateErrorMessage(returned);
}

err.fileName = path;
callback(err);
returned.fileName = path;
callback(returned);
}
};
6 changes: 6 additions & 0 deletions test/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"extends": "../.eslintrc.json",
"env": {
"mocha": true
}
}
3 changes: 1 addition & 2 deletions test/fs.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const assert = require('assert');
const fs = require('fs');
const {bundle, run, assertBundleTree} = require('./utils');
const {bundle, run} = require('./utils');

describe('fs', function() {
it('should inline a file as a string', async function() {
Expand Down
Loading