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.

Sign up for GitHub

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

Jump to bottom

Log gulp error to Chart.js #5143

Merged
merged 13 commits into from Jan 13, 2018
Merged

Log gulp error to Chart.js #5143

Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
142e19d
Log errors and skip for buildTask
loicbourgois Jan 13, 2018
58bb9fe
Write gulp error to Chart.js
loicbourgois Jan 13, 2018
2414daa
Remove unused require
loicbourgois Jan 13, 2018
08cf096
Remove error + Proper require fs
loicbourgois Jan 13, 2018
d2327ef
Fix newline
loicbourgois Jan 13, 2018
ecffc82
Refactor
loicbourgois Jan 13, 2018
e490438
Put back browser errors
loicbourgois Jan 13, 2018
e21587b
Use options
loicbourgois Jan 13, 2018
ae0e7e7
Fix intentional error
loicbourgois Jan 13, 2018
c8388d6
Use yargs + Refactor
loicbourgois Jan 13, 2018
251fcc3
remove space
loicbourgois Jan 13, 2018
0b39c03
Fefactor
loicbourgois Jan 13, 2018
6ad9149
Use booleans
loicbourgois Jan 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 24 additions & 2 deletions gulpfile.js
View file
Open in desktop
Expand Up @@ -20,6 +20,8 @@ var collapse = require('bundle-collapser/plugin');
var argv = require('yargs').argv
var path = require('path');
var package = require('./package.json');
var fs = require('fs');
Copy link
Member

Choose a reason for hiding this comment

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

[nit] can we move var fs before var package since the later one is a local file.

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
while i'm at it, is there any logic in the order for the non local dependencies ? alphabetical, date of addition ?

Copy link
Member

Choose a reason for hiding this comment

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

No guideline for the order because some require() may need other ones. I'm used to gather related dependencies (e.g. gulp-*) and declare the package.json at the end.

var minimist = require('minimist');

var srcDir = './src/';
var outDir = './dist/';
Expand All @@ -34,6 +36,9 @@ var header = "/*!\n" +
" * https://github.com/chartjs/Chart.js/blob/master/LICENSE.md\n" +
" */\n";

var options = minimist(process.argv.slice(2));
Copy link
Member

Choose a reason for hiding this comment

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

We already depend on yargs, no need for minimist:

var argv = require('yargs')
  .option('force-output', {default: 'false'})
  .option('silent-errors', {default: 'false'})
  .option('verbose', {default: 'false'})
  .argv;

// ...

if (argv.forceOutput) { ... }
if (argv.silentErrors) { ... }

util.log("Gulp running with options: "+JSON.stringify(options, null, 2));
Copy link
Member

Choose a reason for hiding this comment

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

if (argv.verbose) {
  util.log("Gulp running with options: " + JSON.stringify(argv, null, 2));
}


gulp.task('bower', bowerTask);
gulp.task('build', buildTask);
gulp.task('package', packageTask);
Expand Down Expand Up @@ -79,9 +84,25 @@ function bowerTask() {

function buildTask() {

var errorHandler = function (err) {
util.log(util.colors.red('[Error]'), err.toString());
Copy link
Member

Choose a reason for hiding this comment

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

I would move that line before this.emit('end');, it's only useful when ignoring exceptions

if(options['force-output']) {
var browserError = 'console.error("Gulp: ' + err.toString() + '")';
['Chart', 'Chart.min', 'Chart.bundle', 'Chart.bundle.min'].forEach(function(fileName) {
fs.writeFileSync(outDir+fileName+'.js', browserError);
});
}
if(options['silent-errors']) {
this.emit('end');
} else {
throw err;
}
}

var bundled = browserify('./src/chart.js', { standalone: 'Chart' })
.plugin(collapse)
.bundle()
.on('error', errorHandler)
.pipe(source('Chart.bundle.js'))
.pipe(insert.prepend(header))
.pipe(streamify(replace('{{ version }}', package.version)))
Expand All @@ -96,6 +117,7 @@ function buildTask() {
.ignore('moment')
.plugin(collapse)
.bundle()
.on('error', errorHandler)
.pipe(source('Chart.js'))
.pipe(insert.prepend(header))
.pipe(streamify(replace('{{ version }}', package.version)))
Expand Down Expand Up @@ -135,15 +157,15 @@ function lintTask() {
// NOTE(SB) codeclimate has 'complexity' and 'max-statements' eslint rules way too strict
// compare to what the current codebase can support, and since it's not straightforward
// to fix, let's turn them as warnings and rewrite code later progressively.
var options = {
var eslintOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

No need to rename this variable, we can keep var argv = ...

rules: {
'complexity': [1, 10],
'max-statements': [1, 30]
}
};

return gulp.src(files)
.pipe(eslint(options))
.pipe(eslint(eslintOptions))
.pipe(eslint.format())
.pipe(eslint.failAfterError());
}
Expand Down