Log gulp error to Chart.js #5143

loicbourgois · 2018-01-13T11:05:35Z

If gulp encounters an error when building, it just stops, which means the Chart.js files are not updated with the error, and that gulp would have to be restarted manually.
This PR takes care of it.

Gulp log before

[12:14:33] Starting 'build'...
events.js:136
      throw er; // Unhandled 'error' event
      ^

Error: Parsing file /home/user/Chart.js/src/chart.js: Unexpected token (5:5)
    at Deps.parseDeps (/home/user/Chart.js/node_modules/module-deps/index.js:481:28)
    at getDeps (/home/user/Chart.js/node_modules/module-deps/index.js:414:40)
    ...

Gulp log after

[12:10:29] Finished 'default' after 20 μs
[12:10:46] Starting 'build'...
[12:10:46] [Error] Error: Parsing file /home/user/Chart.js/src/chart.js: Unexpected token (5:5)
[12:10:46] [Error] Error: Parsing file /home/user/Chart.js/src/chart.js: Unexpected token (5:5)
[12:10:46] Finished 'build' after 42 ms

It's possible to have the full trace using simply util.log(err)

+ Add intentional error to core to check if travis fails

simonbrunel

Can you post part of the log of the error before and after this change?

simonbrunel · 2018-01-13T11:21:26Z

gulpfile.js

+    .on('error', function (err) {
+      util.log(util.colors.red('[Error]'), err.toString())
+      fs.writeFileSync(outDir+'Chart.bundle.js', 'console.error("Gulp: ' + err.toString() + '")');
+      fs.writeFileSync(outDir+'Chart.bundle.min.js', 'console.error("Gulp: ' + err.toString() + '")');


I would not alter Chart.bundle(.min)?.js, simply log the error to console.

simonbrunel · 2018-01-13T11:23:24Z

gulpfile.js

@@ -96,6 +102,12 @@ function buildTask() {
    .ignore('moment')
    .plugin(collapse)
    .bundle()
+    .on('error', function (err) {


Can we avoid code duplication by refactoring this method:

var errorHandler = function (err) { util.log(util.colors.red('[Error]'), err.toString()); this.emit('end'); } // ... .bundle() .on('error', errorHandler) // ... .on('error', errorHandler)

loicbourgois · 2018-01-13T12:25:26Z

I added the logs in the top comment

simonbrunel · 2018-01-13T12:31:15Z

I'm reading again your description: does that mean the gulp build process doesn't fail anymore in case of parsing errors?

loicbourgois · 2018-01-13T12:57:03Z

gulpfile.js

+    fs.writeFileSync(outDir+'Chart.js', browserError);
+    fs.writeFileSync(outDir+'Chart.min.js', browserError);
+    fs.writeFileSync(outDir+'Chart.bundle.js', browserError);
+    fs.writeFileSync(outDir+'Chart.bundle.min.js', browserError);


@simonbrunel if possible, i would like to be able to see the errors directly in the browser. I prefer to work in the editor and the browser only, and not have to check the terminal.

loicbourgois · 2018-01-13T12:59:32Z

Yes, gulp continues to run.
If it's a problem, maybe we could add a task build-dev, that wouldn't fail completely on errors.

loicbourgois · 2018-01-13T13:05:30Z

Maybe the travis log could be helpful https://travis-ci.org/chartjs/Chart.js/builds/328424793

simonbrunel · 2018-01-13T13:11:40Z

These changes are not safe: first the build task must fail and return an error code in order to stop the Travis build and report the failed status. In that case, tests should not run, neither other tasks such as docs, package, etc. Then, in case of a failing build, no artifact should be generated because that could confuse some other tasks and deploy unwanted files.

I'm not fan of the build-dev task because it will enforce your workflow, but also because it duplicates logic and code. Instead, I would adopt another approach by introducing new build options:

--force-output: that would output the error in the dist files
--silent-errors: that would prevent the process to fail

Indeed, the Travis build fails, but that's not controlled, which is a door open on more issues.

simonbrunel · 2018-01-13T13:20:06Z

Though, I plan to refactor the gulp file for a better watch logic and thus make sure it keeps watching on error (so that would deprecate --silent-errors).

loicbourgois · 2018-01-13T14:25:15Z

I followed your advice and used options.
With your plan to refactor, is it still ok to keep --silent-errors for now ?
Also, should I update contributing.md#building-and-testing ?

simonbrunel

We can keep --silent-errors for now.

I don't think I would update contributing.md right now because with the number of options increasing, it will be complicated to maintain. Instead, we will use the yargs help method (gulp --help), which is planned in the upcoming refactor and thus another PR.

simonbrunel · 2018-01-13T14:27:00Z

gulpfile.js

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


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) { ... }

simonbrunel · 2018-01-13T14:29:34Z

gulpfile.js

@@ -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 = {


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

simonbrunel · 2018-01-13T14:32:35Z

gulpfile.js

@@ -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');


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

yes
while i'm at it, is there any logic in the order for the non local dependencies ? alphabetical, date of addition ?

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.

simonbrunel · 2018-01-13T14:40:04Z

gulpfile.js

@@ -79,9 +84,25 @@ function bowerTask() {

 function buildTask() {

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


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

simonbrunel · 2018-01-13T14:46:58Z

gulpfile.js

@@ -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));
+util.log("Gulp running with options: "+JSON.stringify(options, null, 2));


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

simonbrunel · 2018-01-13T15:33:19Z

gulpfile.js

@@ -17,8 +17,13 @@ var browserify = require('browserify');
 var source = require('vinyl-source-stream');
 var merge = require('merge-stream');
 var collapse = require('bundle-collapser/plugin');
-var argv  = require('yargs').argv
+var argv  = require('yargs')


Though, since this one will grow, I would keep it outside the others for a better readability of dependencies :)

Maybe:

var collapse = require('bundle-collapser/plugin'); var yargs = require('yargs'); var path = require('path'); var package = require('./package.json'); var argv = yargs .option('force-output', {default: 'false'}) // .. .argv;

or like that: https://github.com/chartjs/chartjs-plugin-datalabels/blob/master/gulpfile.js#L20

but I think I prefer the first solution: var yargs = require('yargs');

Yes, I think it makes more sense to keep all the dependencies small and together.

simonbrunel · 2018-01-13T15:55:13Z

gulpfile.js

 var package = require('./package.json');

+var argv = yargs
+  .option('force-output', {default: 'false'})


My bad, I think it should be {default: false} (boolean instead of string)

simonbrunel

Thanks @loicbourgois (and sorry for all these iterations)

* Log errors and skip for buildTask * Write gulp error to Chart.js + Add intentional error to core to check if travis fails * Remove unused require * Remove error + Proper require fs * Fix newline * Refactor * Put back browser errors * Use options * Fix intentional error * Use yargs + Refactor * remove space * Fefactor * Use booleans

loicbourgois added 5 commits January 13, 2018 09:57

Log errors and skip for buildTask

142e19d

Write gulp error to Chart.js

58bb9fe

+ Add intentional error to core to check if travis fails

Remove unused require

2414daa

Remove error + Proper require fs

08cf096

Fix newline

d2327ef

simonbrunel requested changes Jan 13, 2018

View reviewed changes

simonbrunel added the type: infrastructure label Jan 13, 2018

loicbourgois added 2 commits January 13, 2018 12:46

Refactor

ecffc82

Put back browser errors

e490438

loicbourgois commented Jan 13, 2018

View reviewed changes

Use options

e21587b

Fix intentional error

ae0e7e7

simonbrunel requested changes Jan 13, 2018

View reviewed changes

loicbourgois added 2 commits January 13, 2018 15:26

Use yargs + Refactor

c8388d6

remove space

251fcc3

simonbrunel reviewed Jan 13, 2018

View reviewed changes

Fefactor

0b39c03

simonbrunel reviewed Jan 13, 2018

View reviewed changes

Use booleans

6ad9149

simonbrunel approved these changes Jan 13, 2018

View reviewed changes

simonbrunel requested a review from etimberg January 13, 2018 16:33

etimberg approved these changes Jan 13, 2018

View reviewed changes

etimberg merged commit e585c75 into chartjs:master Jan 13, 2018

loicbourgois deleted the gulp branch January 13, 2018 17:36

simonbrunel added this to the Version 2.7.2 milestone Jan 13, 2018

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log gulp error to Chart.js #5143

Log gulp error to Chart.js #5143

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel left a comment •

edited

Loading

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel commented Jan 13, 2018 •

edited

Loading

loicbourgois Jan 13, 2018

loicbourgois commented Jan 13, 2018

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel commented Jan 13, 2018

simonbrunel commented Jan 13, 2018

loicbourgois commented Jan 13, 2018

simonbrunel left a comment

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018

loicbourgois Jan 13, 2018

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018 •

edited

Loading

simonbrunel Jan 13, 2018

simonbrunel Jan 13, 2018 •

edited

Loading

loicbourgois Jan 13, 2018

simonbrunel Jan 13, 2018

simonbrunel left a comment

Log gulp error to Chart.js #5143

Log gulp error to Chart.js #5143

Conversation

loicbourgois commented Jan 13, 2018 • edited Loading

Gulp log before

Gulp log after

simonbrunel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicbourgois commented Jan 13, 2018 • edited Loading

simonbrunel commented Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

loicbourgois commented Jan 13, 2018

loicbourgois commented Jan 13, 2018 • edited Loading

simonbrunel commented Jan 13, 2018

simonbrunel commented Jan 13, 2018

loicbourgois commented Jan 13, 2018

simonbrunel 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

simonbrunel Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel left a comment

Choose a reason for hiding this comment

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel left a comment •

edited

Loading

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel commented Jan 13, 2018 •

edited

Loading

loicbourgois commented Jan 13, 2018 •

edited

Loading

simonbrunel Jan 13, 2018 •

edited

Loading

simonbrunel Jan 13, 2018 •

edited

Loading