-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Changes from all commits
96469cb
c850a1e
e103804
c574900
fe571bf
8b492f2
d2738bd
d919fa9
526afc8
26aabe0
dd55256
944f360
ec0e1c6
9c1215c
437b385
2e81db0
7a0ad22
7b1714e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"extends": "eslint:recommended", | ||
"parserOptions": { | ||
"ecmaVersion": 8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, babel seem used everywhere in parcel. It allows to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,5 +4,7 @@ node_js: | |
# - '6' | ||
- '8' | ||
cache: yarn | ||
script: yarn test | ||
script: | ||
- yarn test | ||
- yarn lint | ||
sudo: false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created this one in the I propose not to deactivate it everywhere, because usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"extends": "../.eslintrc.json", | ||
"rules": { | ||
"no-console": 0 | ||
} | ||
} |
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'); | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it normal that |
||
const Path = require('path'); | ||
const Bundle = require('./Bundle'); | ||
const {FSWatcher} = require('chokidar'); | ||
|
@@ -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'); | ||
|
||
/** | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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('.')) { | ||
|
@@ -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; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,7 +70,9 @@ class FSCache { | |
try { | ||
await fs.unlink(this.getCacheFile(filename)); | ||
this.invalidated.delete(filename); | ||
} catch (err) {} | ||
} catch (err) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} | ||
} | ||
|
||
|
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]+:)/; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. The |
||
const IMPORT_RE = /@import/; | ||
const PROTOCOL_RE = /^[a-z]+:/; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I needed to specify the context used by files in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll do it :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, and I also replaced one |
||
"extends": "../.eslintrc.json", | ||
"parserOptions": { | ||
"ecmaVersion": 5 | ||
}, | ||
"env": { | ||
"browser": true | ||
}, | ||
"rules": { | ||
"no-global-assign": 1 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]); | ||
} | ||
|
@@ -22,7 +22,7 @@ function getBundleURL() { | |
} | ||
|
||
function getBaseURL(url) { | ||
return ('' + url).replace(/^((?:https?|file|ftp):\/\/.+)\/[^\/]+$/, '$1') + '/'; | ||
return ('' + url).replace(/^((?:https?|file|ftp):\/\/.+)\/[^/]+$/, '$1') + '/'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. The |
||
} | ||
|
||
exports.getBundleURL = getBundleURLCached; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,6 @@ function reloadCSS() { | |
|
||
cssTimeout = null; | ||
}, 50); | ||
}; | ||
} | ||
|
||
module.exports = reloadCSS; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ class Packager { | |
|
||
async start() {} | ||
|
||
// eslint-disable-next-line no-unused-vars | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. The |
||
|
||
module.exports = function(url) { | ||
return isURL(url) || ANCHOR_REGEXP.test(url) || SCHEME_REGEXP.test(url); | ||
|
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -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); | ||
} | ||
}; |
There was a problem hiding this comment.
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