Setup linter for HTML #5195

loicbourgois · 2018-01-28T11:42:03Z

A comment from @benmccann made me realise the Javascript linter was not checking the files in /samples. Comment is here.

This is a PR to setup a linter for HTML files using html plugin for eslint and htmllint.

The rules for htmllint can be modified in .htmllintrc.js . List of options is available here.

In gulpfile.js I added the option lint-script-elements to lint (or not) the HTML files in lintTask().
Enabling this option gives 4166 errors.

$ gulp lint --lint-script-elements
✖ 4230 problems (4166 errors, 64 warnings)
  3481 errors, 0 warnings potentially fixable with the `--fix` option.

What would be the best way to update the examples : file by file, folder by folder, all at once ?

simonbrunel

Thanks Loic for tackling these tasks

[ ... edited ... ]

I didn't review the "htmllint" related changes.

What would be the best way to update the examples : file by file, folder by folder, all at once ?

All at once, in this PR

4230 problems (4166 errors, 64 warnings)

That's a lot, what the main reason for so many errors? space vs tabs?

Edit(SB): removed suggestion to split that PR after our Slack talk

simonbrunel · 2018-01-28T12:53:22Z

gulpfile.js


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


If we decide to lint our samples, then it should be part of the regular linting process. That why I would not add this --lint-script-element argument and keep script simpler by always parsing those HTML files.

simonbrunel · 2018-01-28T12:55:51Z

gulpfile.js

  .option('silent-errors', {default: false})
  .option('verbose', {default: false})
  .argv

 var srcDir = './src/';
 var outDir = './dist/';

+var htmlFiles = [


This is equivalent to './samples/**/*.html'. I would put that one line directly where it's used (as we do for the rest of the gulpfile) (Note: I'm removing srcDir/outDir in the upcoming gulp rewrite).

simonbrunel · 2018-01-28T12:56:55Z

gulpfile.js

@@ -159,6 +169,9 @@ function lintTask() {
    'src/**/*.js',
    'test/**/*.js'
  ];
+  if (argv.lintScriptElements) {
+    files = files.concat(htmlFiles);


Per my previous comment:

var files = [ 'samples/**/*.html', 'samples/**/*.js', 'src/**/*.js', 'test/**/*.js' ];

loicbourgois · 2018-01-28T13:30:18Z

Some more details about the errors :

2606 Expected indentation of x tab(s)
603 Strings must use singlequote
598 is not defined
- 499 'randomScalingFactor' is not defined
- 80 'Chart' is not defined
- 19 other
359 other

loicbourgois · 2018-01-28T13:53:07Z

I think HTML files should at least be checked for indentation. It will be a lot easier to have consistency between the content of <script> elements and the rest of the DOM.

@simonbrunel I'm not sure how to discuss this on slack : I tried the badge on the readme, and was redirected to https://chart-js-automation.herokuapp.com/ with a message "There's nothing here, yet.".

simonbrunel · 2018-01-28T14:02:46Z

I think HTML files should at least be checked for indentation

Let's talk about that on Slack / another PR since I doubt ESLint can handle that.

https://chart-js-automation.herokuapp.com/

Right, it's broken but I don't have access to fix it (@etimberg ?)

simonbrunel · 2018-01-28T14:12:10Z

@tannerlinsley actually, that might be you that setup the heroku app?
@loicbourgois I sent you an invitation to join Slack

loicbourgois · 2018-01-28T21:16:37Z

samples/advanced/progress-bar.html

-                }
-            }
-        };
+		<div style="width: 75%;">


need to find a rule to not allow 2 tabs

loicbourgois · 2018-01-28T21:18:48Z

samples/advanced/data-labelling.html

+				// Define a plugin to provide data labels
+				Chart.plugins.register({
+					afterDatasetsDraw: function(chart/* , easing */) { // TODO : remove easing ?
+						// To only draw at the end of animation, check for easing === 1


comment and code would be out of sync

I would remove both the variable and the comment

loicbourgois · 2018-01-28T21:21:16Z

samples/charts/line/stepped.html

 	function createConfig(details, data) {
 		return {
 			type: 'line',
 			data: {
 				labels: ['Day 1', 'Day 2', 'Day 3', 'Day 4', 'Day 5', 'Day 6'],
 				datasets: [{
-					label: 'steppedLine: ' + ((typeof(details.steppedLine) === 'boolean') ? details.steppedLine : `'${details.steppedLine}'`),
+					label: 'steppedLine: ' + ((typeof (details.steppedLine) === 'boolean') ? details.steppedLine : details.steppedLine), // TODO : check this


not sure what's going on here

typeof as operator without parenthesis?

no, i mean why do we have :

? details.steppedLine : `'${details.steppedLine}'`

I think typeof(foo) is invalid because it's an operator so should not be used with parenthesis. We should write typeof foo, not typeof (foo) (with the extra space).

Anyway, you right, this line is weird, could be: label: 'steppedLine: ' + details.steppedLine

loicbourgois · 2018-01-28T21:23:20Z

samples/scales/time/line-point-data.html

@@ -122,10 +122,13 @@
 			window.myLine.update();
 		});

-		document.getElementById('addData').addEventListener('click', function() {
+		document.getElementById('addData').addEventListener('click', function() { // TODO : fix issue with addData


This seems broken

I would update the comment to link to the issue. It's not clear from the TODO right now what is supposed to be fixed

simonbrunel · 2018-01-29T21:51:25Z

It's very hard to review this PR, many modifications that are obfuscated by the indentation change. It might have been better to submit a PR that only fixes the indentation without touching the rest of the code.

simonbrunel

A few feedback but can't review the whole PR for the reasons explained in my previous comment (we may need to move the indentation changes in another PR). Also, the indentation doesn't seem correct for the HTML part.

simonbrunel · 2018-01-29T21:53:19Z

gulpfile.js

-      'max-statements': [1, 30]
-    }
+      'max-statements': [1, 30],
+      'no-new': 0, // TODO : is this allowed ?


It's not allow to call new without storing the result. We should fix these cases instead of disabling this rule.

I tried var chart = new Chart(...) but then it triggers an error because chart is not used.

Makes sense, let's disable no-new then

simonbrunel · 2018-01-29T21:56:38Z

gulpfile.js

+      'Chart',
+      'moment',
+      'randomScalingFactor',
+    ]


These globals should not be enabled for the whole repository but scoped to the samples directory in its own samples/.eslintrc.yml.

simonbrunel · 2018-01-29T21:57:18Z

gulpfile.js

+      // 125:5  error  Identifier 'samples_filler_analyser' is not in camel case  camelcase
+      // /Chart.js/samples/charts/area/radar.html
+      // 103:5  error  Identifier 'samples_filler_analyser' is not in camel case  camelcase
+      'camelcase': 0,


Does it pass with 'samples-filler-analyser'? else this rule should be moved in samples/.eslintrc.yml

simonbrunel · 2018-01-29T21:59:34Z

.htmllintrc.js

@@ -0,0 +1,10 @@
+module.exports = {


Does htmllint support YAML config? It's usually more readable and easy to manipulate but also consistent with our other config files.

simonbrunel · 2018-01-29T22:00:20Z

.htmllintrc.js

+		'indent-style': 'tabs',
+		'attr-quote-style': 'double',
+		'spec-char-escape': false,
+		'attr-bans': ['align', 'background', 'bgcolor', 'border', 'frameborder', 'longdesc', 'marginwidth', 'marginheight', 'scrolling', 'syle'/*, 'width'*/],


I think there is a typo to syle but do we really need these explicit bans?

I took the default option and commented the attributes triggering errors.
See https://github.com/htmllint/htmllint/wiki/Options#attr-bans

I see, then you can remove the 2 last values ('syle' which was initially 'style' and 'width')

simonbrunel · 2018-01-29T22:08:42Z

gulpfile.js

 gulp.task('docs', docsTask);
-gulp.task('test', ['lint', 'validHTML', 'unittest']);
+gulp.task('test', ['lint', 'lint-html', 'validHTML', 'unittest']);


I think htmllint do the same job as gulp-html-validator (unmaintained?), so we should remove the 'validHTML' task and associated node dependencies in package.json.

simonbrunel · 2018-01-29T22:09:36Z

gulpfile.js

@@ -157,7 +160,8 @@ function lintTask() {
  var files = [
    'samples/**/*.js',
    'src/**/*.js',
-    'test/**/*.js'
+    'test/**/*.js',
+    './samples/**/*.html'


For consistency:

var files = [ 'samples/**/*.html' 'samples/**/*.js', 'src/**/*.js', 'test/**/*.js' ]

simonbrunel · 2018-01-29T22:10:06Z

package.json

@@ -17,13 +17,15 @@
    "coveralls": "^3.0.0",
    "eslint": "^4.9.0",
    "eslint-config-chartjs": "^0.1.0",
+    "eslint-plugin-html": "^4.0.2",
    "gitbook-cli": "^2.3.2",
    "gulp": "3.9.x",
    "gulp-concat": "~2.6.x",
    "gulp-connect": "~5.0.0",
    "gulp-eslint": "^4.0.0",
    "gulp-file": "^0.3.0",
    "gulp-html-validator": "^0.0.5",


Should be removed (see previous comment)

simonbrunel · 2018-01-29T22:13:12Z

samples/advanced/data-labelling.html

-        -ms-user-select: none;
-    }
-    </style>
+		<title>Labelling Data Points</title>


All files are now indented with 2 tabs instead of 1 for the HTML part

simonbrunel · 2018-01-29T22:16:18Z

samples/charts/area/line-boundaries.html

@@ -18,14 +18,14 @@
 		<div class="wrapper col-2"><canvas id="chart-3"></canvas></div>

 		<div class="toolbar">
-			<button onclick="toggleSmooth(this)">Smooth</button>
-			<button onclick="randomize(this)">Randomize</button>
+			<button id="toggleSmooth">Smooth</button>


What the reason of this change?

Linting JS in HTML attributes is not supported yet, so I had to move event handling in a <script> element.
I submitted a PR to add attributes linting support : BenoitZugmeyer/eslint-plugin-html#87

I really prefer the other syntax and was about to update other samples to remove these addEventListener. Can we revert these changes?

If the PR is accepted, yes.
If not, we have 2 choices :

use onclick() and add a rule to ignore no-undef-var for /samples

use addEventListener

no-undef-var: 0 in samples/.eslintrc.yml :)

Alternatively (and probably better), we can disable this rule inline before each method:

// eslint-disable-next-line no-undef-var function randomize() { //...

loicbourgois · 2018-01-29T22:53:35Z

@simonbrunel you can add ?w=1 to the url to ignore whitespace in diffs.
see camsong/chrome-github-mate#5
Not sure how well it works.
I'm happy to proceed with multiple PR if it's easier.

simonbrunel · 2018-01-29T23:14:10Z

That's much better with ?w=1, thanks for the tip!

simonbrunel · 2018-01-30T09:47:19Z

The only con with ?w=1: you can't add review comments

benmccann · 2018-02-01T03:20:08Z

samples/charts/area/line-datasets.html

@@ -152,6 +152,18 @@
 			});
 			chart.update();
 		}
+
+		document.getElementById('togglePropagate').onclick = function() {


Maybe better to use addEventListener('click'?

Better to keep the onclick event handler inline, as DOM attribute (previous code)

benmccann · 2018-02-01T03:26:40Z

samples/scales/time/line-point-data.html

 			if (config.data.datasets.length > 0) {
-				var numTicks = myLine.scales['x-axis-0'].ticksAsTimestamps.length;
-				var lastTime = numTicks ? moment(myLine.scales['x-axis-0'].ticksAsTimestamps[numTicks - 1]) : moment();
+				// var numTicks = window.myLine.scales['x-axis-0'].ticksAsTimestamps.length; // TODO : check


are these commented out because of the issue in referenced in the TODO?

This reverts commit 3746bad.

loicbourgois · 2018-02-01T20:34:47Z

Fixed indentation for most files (2 tabs -> 1 tab).
However, I did not find any option to check for this with htmllint. So I added the functionality locally + submitted a PR.
This means that this kind of indentation error will not get caught for now.

simonbrunel

Looks really good, one extra fix (plugin id) and a few minor changes.

I think it's fine if we don't detect extra indentation until htmllint/htmllint#227 is merged.

simonbrunel · 2018-02-01T21:22:56Z

gulpfile.js

 gulp.task('docs', docsTask);
-gulp.task('test', ['lint', 'validHTML', 'unittest']);


We should also remove the validHtml associated function

simonbrunel · 2018-02-01T21:23:19Z

package.json

    "gitbook-cli": "^2.3.2",
    "gulp": "3.9.x",
    "gulp-concat": "~2.6.x",
    "gulp-connect": "~5.0.0",
    "gulp-eslint": "^4.0.0",
    "gulp-file": "^0.3.0",
-    "gulp-html-validator": "^0.0.5",
+    "gulp-htmllint": "0.0.15",


"^0.0.15"

simonbrunel · 2018-02-01T21:25:23Z

samples/charts/area/line-datasets.html

@@ -122,7 +122,7 @@
 				filler: {
 					propagate: false
 				},
-				samples_filler_analyser: {
+				'samples-filler-analyser': {


You also need to change the plugin id

loicbourgois · 2018-02-01T21:57:41Z

@simonbrunel what about no-unused-vars for inline events ?
Should we use comments to disable it for now and revert in another PR once BenoitZugmeyer/eslint-plugin-html#87 is merged ?

simonbrunel · 2018-02-02T09:00:01Z

You right, it's not no-undef-var but no-unused-vars. I'm fine with disabling locally in comment (// eslint-disable-next-line no-unused-var) or globally at the samples/.eslintrc.yml.

loicbourgois · 2018-02-02T13:10:55Z

Should be ready for merge

loicbourgois added 5 commits January 27, 2018 22:24

Add htmllint

3ff10ac

Add htmllint

13cd5b0

Setup html plugin for eslint

fddb06f

Add option to lint script tags

6f821d7

Rename option

83822eb

simonbrunel added the type: infrastructure label Jan 28, 2018

simonbrunel requested changes Jan 28, 2018

View reviewed changes

loicbourgois added 10 commits January 28, 2018 18:13

Convert spaces to tabs

fd5232a

Setup eslint fix

7cfcf5a

setup

1b591b1

Autofix eslint

9f130a2

Rollback before autofix

c640e69

Remove most errors

098c133

Correct introduced errors

71792f1

Check samples

70822cf

Fix lint

f8f119a

Add TODO comments

8183537

loicbourgois commented Jan 28, 2018

View reviewed changes

simonbrunel requested changes Jan 29, 2018

View reviewed changes

loicbourgois added 2 commits January 31, 2018 21:57

Use default config file for htmllint

8ac4864

Fail on error

ccd9cab

benmccann reviewed Feb 1, 2018

View reviewed changes

loicbourgois added 8 commits February 1, 2018 11:14

Remove error

1fc07d2

Remove unecessary comment

8e4c315

Add link to issue for add data bug

4675e85

Clean gulpfile

9b59b55

Revert to inline event handler

9bb89f4

Disable no-unused-vars for inline events

3746bad

Revert "Disable no-unused-vars for inline events"

261085f

This reverts commit 3746bad.

Indentation

1474880

simonbrunel requested changes Feb 1, 2018

View reviewed changes

simonbrunel requested a review from etimberg February 1, 2018 21:28

loicbourgois added 3 commits February 1, 2018 22:15

Remove old htmlTask

57fc01c

Fix package.json

4a2ff8c

Fix plugin id

03321b0

eslint-disable-next-line no-unused-vars

58db09d

etimberg approved these changes Feb 3, 2018

View reviewed changes

simonbrunel approved these changes Feb 3, 2018

View reviewed changes

simonbrunel merged commit 182270e into chartjs:master Feb 3, 2018

simonbrunel added this to the Version 2.7.2 milestone Feb 3, 2018

loicbourgois deleted the html-linter branch February 3, 2018 12:56

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

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Setup HTML and JS linters for samples (chartjs#5195)

a8482cf

		gulp.task('docs', docsTask);
		gulp.task('test', ['lint', 'validHTML', 'unittest']);

Setup linter for HTML #5195

Setup linter for HTML #5195

Conversation

loicbourgois commented Jan 28, 2018 • edited

simonbrunel left a comment • edited

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

loicbourgois commented Jan 28, 2018 • edited

loicbourgois commented Jan 28, 2018

simonbrunel commented Jan 28, 2018

simonbrunel commented Jan 28, 2018

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

loicbourgois Jan 30, 2018 • edited

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 commented Jan 29, 2018 • edited

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

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 30, 2018 • edited

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

loicbourgois commented Jan 29, 2018

simonbrunel commented Jan 29, 2018 • edited

simonbrunel commented Jan 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicbourgois commented Feb 1, 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

loicbourgois commented Feb 1, 2018

simonbrunel commented Feb 2, 2018

loicbourgois commented Feb 2, 2018

loicbourgois commented Jan 28, 2018 •

edited

simonbrunel left a comment •

edited

loicbourgois commented Jan 28, 2018 •

edited

loicbourgois Jan 30, 2018 •

edited

simonbrunel commented Jan 29, 2018 •

edited

simonbrunel Jan 30, 2018 •

edited

simonbrunel commented Jan 29, 2018 •

edited

simonbrunel commented Jan 30, 2018 •

edited