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 all commits
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
20 changes: 20 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# 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/dynamic-references-raw/index.js
/test/integration/dynamic-references-raw/local.js
/test/integration/hmr-dynamic/index.js

# Generated by the build
lib
dist
/test/dist
/test/input

coverage
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 appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ test_script:
- yarn --version
# run tests
- yarn test
# run ESlint
- yarn lint

cache:
- "%LOCALAPPDATA%\\Yarn"
Expand Down
6 changes: 6 additions & 0 deletions bin/.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.

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)

Choose a reason for hiding this comment

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

It was a great time with you

"extends": "../.eslintrc.json",
"rules": {
"no-console": 0
}
}
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"babel-preset-env": "^1.6.1",
"coffeescript": "^2.0.3",
"cross-env": "^5.1.1",
"eslint": "^4.13.0",
"husky": "^0.14.3",
"less": "^2.7.2",
"lint-staged": "^6.0.0",
Expand All @@ -65,7 +66,8 @@
"format": "prettier --write './{src,bin,test}/**/*.{js,json,md}'",
"build": "babel src -d lib",
"prepublish": "yarn build",
"precommit": "lint-staged",
"precommit": "npm run lint && lint-staged",
"lint": "eslint .",
"postinstall":
"node -e \"console.log('\\u001b[35m\\u001b[1mLove Parcel? You can now donate to our open collective:\\u001b[22m\\u001b[39m\\n > \\u001b[34mhttps://opencollective.com/parcel/donate\\u001b[0m')\""
},
Expand Down
6 changes: 6 additions & 0 deletions src/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"extends": "../.eslintrc.json",
"rules": {
"no-console": 0
}
}
5 changes: 1 addition & 4 deletions src/Asset.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,7 @@ class Asset {
from = this.name;
}

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

let resolved = path.resolve(path.dirname(from), url).replace(/[?#].*$/, '');
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
16 changes: 8 additions & 8 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 All @@ -13,7 +12,6 @@ const {EventEmitter} = require('events');
const Logger = require('./Logger');
const PackagerRegistry = require('./packagers');
const localRequire = require('./utils/localRequire');
const customErrors = require('./utils/customErrors');
const config = require('./utils/config');

/**
Expand Down Expand Up @@ -273,8 +271,10 @@ 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}'`;

// Add absolute path to the error message if the dependency specifies a relative path
if (dep.name.startsWith('.')) {
Expand All @@ -285,13 +285,13 @@ class Bundler extends EventEmitter {
// 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
4 changes: 3 additions & 1 deletion src/FSCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,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
4 changes: 1 addition & 3 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');
const getPort = require('get-port');
const serverErrors = require('./utils/customErrors').serverErrors;
Expand Down Expand Up @@ -66,7 +64,7 @@ async function serve(bundler, port) {
reject(err);
});

server.once('listening', connection => {
server.once('listening', () => {
let addon =
server.address().port !== port
? `- ${bundler.logger.chalk.red(
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
1 change: 0 additions & 1 deletion src/assets/CoffeeScriptAsset.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const JSAsset = require('./JSAsset');
const config = require('../utils/config');
const localRequire = require('../utils/localRequire');

class CoffeeScriptAsset extends JSAsset {
Expand Down
5 changes: 2 additions & 3 deletions src/assets/GlobAsset.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
const Asset = require('../Asset');
const glob = require('glob');
const promisify = require('../utils/promisify');
const globPromise = promisify(glob);
const micromatch = require('micromatch');
const path = require('path');

Expand Down Expand Up @@ -29,7 +27,8 @@ class GlobAsset extends Asset {
.slice(1)
.filter(Boolean)
.reduce((a, p) => a.concat(p.split('/')), []);
let relative = './' + path.relative(path.dirname(this.name), file.normalize('NFC'));
let relative =
'./' + path.relative(path.dirname(this.name), file.normalize('NFC'));
set(matches, parts, relative);
this.addDependency(relative);
}
Expand Down
2 changes: 0 additions & 2 deletions src/assets/HTMLAsset.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
const Asset = require('../Asset');
const posthtml = require('posthtml');
const parse = require('posthtml-parser');
const api = require('posthtml/lib/api');
const path = require('path');
const url = require('url');
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
12 changes: 12 additions & 0 deletions src/builtins/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
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",
"parserOptions": {
"ecmaVersion": 5
},
"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;
4 changes: 2 additions & 2 deletions src/builtins/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const builtins = require('node-libs-browser');
var builtins = require('node-libs-browser');

for (let key in builtins) {
for (var key in builtins) {
if (builtins[key] == null) {
builtins[key] = require.resolve('./_empty.js');
}
Expand Down
19 changes: 11 additions & 8 deletions src/builtins/prelude.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,23 @@ require = (function (modules, cache, entry) {
err.code = 'MODULE_NOT_FOUND';
throw err;
}

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

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

localRequire.resolve = resolve;

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));
}

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

function Module() {
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: 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
3 changes: 1 addition & 2 deletions 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 All @@ -18,7 +17,7 @@ const VARS = {
};

module.exports = {
MemberExpression(node, asset, ancestors) {
MemberExpression(node, asset) {
// Inline environment variables accessed on process.env
if (matchesPattern(node.object, 'process.env')) {
let key = types.toComputedKey(node);
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);
}
};
Loading