Include both CSSLint and stylelint #150

Mottie · 2017-08-13T02:34:44Z

As mentioned in #149, the stylelint bundle file is about 3.25 Mb in size. I'm not sure if that makes this a reasonable change, but I went ahead and did it.

If this PR is accepted, all the locale files will need to be updated in transifex to remove reference to CSSLint.

tophf · 2017-08-13T04:30:00Z

Those who just install styles don't need a better linter and the significant increase of the extension size. Those authors who don't use advanced features provided by StyleLint would not enjoy the increase in the time needed to load and initialize the editor. The solution can be creating an additional editor extension so that Stylus would use it for its edit action. I've already explained all this when StyleLint was suggested.

tophf · 2017-08-13T05:01:13Z

On the other hand, the zip increase is only 500kB -> 1MB, which is kinda tolerable. And the increased load/parse times may not be an issue for those who absolutely need StyleLint. So we can still use this PR if you add an option to use CSSLint or StyleLint in the editor. The corresponding script(s) elements should be added based on the option value. And of course the editor should be able to use both linters.

tophf · 2017-08-13T05:24:46Z

Ah, since there are CodeMirror addons that depend on linter script the entire script loading process should be programmatic, I guess, something like this

narcolepticinsomniac · 2017-08-13T05:41:57Z

Tolerable is a step up from unacceptable. The size increase is pretty drastic for something that'd only end up as an option though, no? I don't see how it's worth it if we're not committed to making it the default. Whether that's worth doing is apparently debatable. How noticeable are any speed issues? I tested briefly, and didn't see anything too out of whack.

I see a few necessary overwrites right off the bat with StyeLint. Whatever is used will need tweaks, but with CSSlint, we're basically forking an abandoned project to maintain locally, whereas StyleLint can be expected to be generally maintained, correct?

Ultimately, you deal with most of the CSSLint bullshit tophf, so it's your call, but I'd think that a size increase this drastic would only be warranted for integral, default functionality, not an optional feature which'll never be utilized by the majority of users.

Mottie · 2017-08-13T06:24:10Z

Stylelint is more actively maintained and also supports a lot of the newer css changes like variables and it'll work with the stylus preprocessor (for issue #134) out of the box. I know the bundle file is huge, but I think with a bit of work removing unnecessary code, we could bring that size down. The problem would be duplicating the modifications on updated versions of Stylelint.

Also, stylelint doesn't include a worker like CSSLint did, so there may be some performance differences.

tophf · 2017-08-13T06:24:58Z

Well, the majority of users don't even open the editor hence my initial reluctance... As for the performance, my i7 PC or yours are probably faster than the average so I'm judging by the numbers in devtools profiler: StyleLint takes as much time to load as the editor itself, linting is several times slower compared to CSSLint. With large styles and slower computers this difference will be noticeable.

tophf · 2017-08-13T06:27:31Z

CSSLint doesn't use service workers, it's just a trivial code cosmetics that makes the script easier to include in a service worker. We don't use service workers as well. The performance difference is inherent because CSSLint is much more simple.

Mottie · 2017-08-13T06:33:29Z

Hmm, maybe we could have a toggle to enable/disable active linting, and a button to check the linting on click. I'll look at it some more tomorrow.

tophf · 2017-08-13T06:34:39Z

For advanced authoring it's better to implement the external editor support. Currently the development of that feature is stalled for unknown reason.

tophf · 2017-08-13T08:02:48Z

To clarify my stance: we can switch to StyleLint as the default linter as long as there's an option to switch to CSSLint. Also, it'd be reasonable to add a config dialog to choose the rules using checkboxes (and, ideally, with an import of a standard StyleLint config format).

narcolepticinsomniac · 2017-08-13T12:53:18Z

If we do end up switching to StyleLint as the default, here's the immediately obvious bugs.

"Write a new style" opens to an "unexpected empty source" error, non-typical selectors without . or # throw errors, errors are thrown pretty randomly for "unexpected longhand/shorthand" for valid code (see Github Dark), and :increment and :decrement throw errors in scrollbar code.

Examples:

g-snapping-carousel {
    color: red;
}
::-webkit-scrollbar-button:horizontal:decrement {
    background-color: transparent;
}

Moz-format import/export are also broken.

I do remember back in the day, using Freestyler on a dual core, I had to disable the persistent validator. Not even sure what they were using, but the CPU usage was a bitch. The ability to revert to CSSLint should suffice, but options to completely disable, or only validate on save, might be useful to some.

Pardon the ignorance, as always, but how efficient is the persistent validator in general? Is it a blunt instrument, constantly parsing the entire code, or is it only checking newly added code that hasn't already been validated? Even then, I would think it could only check for a few errors until matching brackets and then run the gambit. Not suggesting we could or should reinvent the validator, but I'm curious how resource intensive it is needlessly.

tophf · 2017-08-13T13:08:09Z

All linters validate the entire style. I've been thinking today about limiting the area, but it's not trivial so I've postponed the thought until the editor is rewritten.

Mottie · 2017-08-13T13:45:39Z

"Write a new style" opens to an "unexpected empty source" error

I can change the config settings to ignore this. This would also mean modifying the "block-no-empty" rule (e.g. .test {}), but maybe it's better to show a warning instead of an error for this one?

non-typical selectors without . or # throw errors

This is the typical behavior of the linter... it's hard to guess what selectors someone will use. These can be changed to warnings instead of errors.

Errors are thrown pretty randomly for "unexpected longhand/shorthand" for valid code (see Github Dark)

I was testing this update with both GitHub-Dark and Stackoverflow-Dark. The "unexpected longhand/shorthand" issues are actually valid in this case. I could also change these to be warnings.

:increment and :decrement throw errors in scrollbar code.

If I paste the scrollbar code into the stylelint demo, these errors pop up, so it's an issue with stylelint which is interesting since it appears to actually be included in their keyword list. I opened an issue - stylelint/stylelint#2810.

Mottie · 2017-08-13T14:54:21Z

how efficient is the persistent validator in general? Is it a blunt instrument, constantly parsing the entire code, or is it only checking newly added code that hasn't already been validated? Even then, I would think it could only check for a few errors until matching brackets and then run the gambit. Not suggesting we could or should reinvent the validator, but I'm curious how resource intensive it is needlessly.

I didn't extensively look at the edit.js code, but it appears that linting is updated through CodeMirror's set delay value (cm.state.lint.options.delay; which I have no idea where it's set) + 100 ms. I set an initial delay of 200 ms when the editor is first opened because it wasn't showing the issue panel at all without a delay.

It is pretty much a "blunt instrument" otherwise in that it checks the entire section on change; so it isn't too bad when I edit Stackoverflow-Dark with 40+ sections. It isn't too bad when I edit GitHub-Dark either... It doesn't seem that slow on my system, although I do have a older i7 quad core.

it'd be reasonable to add a config dialog to choose the rules using checkboxes (and, ideally, with an import of a standard StyleLint config format).

@tophf Do you mean add settings for standard rules or stylistic rules? or both? Maybe allowing the user to copy/paste their config JSON file into a separate textarea would be easier?

tophf · 2017-08-13T15:30:32Z

I mean whatever makes sense. I don't use StyleLint but I know it has lots of rules which obviously won't suit everyone. In CSSLint we've enabled just four rules. Not sure the other ones are useful but maybe I'll add a config dialog for CSSLint as well. Someday.

Mottie · 2017-08-15T19:18:42Z

@tophf I've been working on a dropdown select on the edit.html page, I can't seem to get the (dynamically) user selected linter to update... When the dropdown is modified, the acmeEventListener in edit.js is called. I added the following entry inside the switch function:

case 'linter':
  if (value !== null && (editors.lastActive || editors[0])) {
    if (prefs.get(el.id) !== value) {
      prefs.set(el.id, value || 'csslint');
    }
    CodeMirror.signal(editors.lastActive || editors[0], "change");
    // save();
  }
  break;

If I signal a change on editors.lastActive || editors[0], I see this error when switching the linter:

foldgutter.js:107 Uncaught TypeError: Cannot read property 'state' of undefined
at onChange (foldgutter.js:107)
at Function.signal (codemirror.js:1175)
at HTMLElement.acmeEventListener (edit.js:357)

The call to save() is supposed to work (ref), but it doesn't appear to update the issues container properly. Only after css content modification does the selected linter correctly report issues.

tophf · 2017-08-15T19:48:45Z

I'd expect you would do something similar to case 'autocompleteOnTyping', but anyway, I hope you can figure it out yourself as I'm not an infallible expert on CodeMirror/whatever.

tophf · 2017-08-16T21:04:01Z

What about "tree shaking" to make the bundle smaller? I find it hard to believe that all that code is needed.

Mottie · 2017-08-17T00:15:10Z

Well, I've tried webpack, browserify and rollup. The first two methods always seems to result in files 3+Mb in size - like it wasn't tree shaking at all. And, I just can't seem to get rollup to include node module files (even when I include rollup-plugin-node-resolve). I'll keep trying.

Mottie · 2017-08-17T20:26:03Z

Ok, I think it's ready for review now... some notes:

I have yet to produce an optimized (tree shaking) version of stylelint
- Currently includes both a full size (3.25Mb) and a .min (1.4Mb - only whitespace & comments removed) version.
- I seem to remember the reviewers at Chrome asking for un-minified files, so it's likely that neither of those files will be the final version.
- I'll set up a repo with my current attempt at tree-shaking stylelint soon - then we can find & fix the problem and have a version that can be easily updated.
A popup allowing modification of stylelint options has been added:
- There is no validation, other than valid JSON on it. Will there need to be more validation, like the checking of valid rules?
- Should these setting be set up with an instance of CodeMirror? If so, should I add the built-in js linter?
- Adding all of the rules from this demo causes some serious lag. I might add an update to use a generator to build the issue list in the header.
All linting functions were split out from the edit.js and moved into lint.js file in the edit folder. Is that a good name, or too redundant & confusing?

tophf · 2017-08-17T20:50:03Z

CodeMirror instance in a popup function like the one used for import-export, yes. And js linter addon (IIRC it's tiny), dynamically loaded of course just like beautifier (script loader may be refactored/promisified). Minimized vendor code is okay at least in case of well-known libraries, also its parsing and compiling are usually noticeably faster. I'll review the code tomorrow.

tophf

I always see an error in the console for any style, didn't investigate though.

lint.js:154 Uncaught TypeError: Cannot read property 'children' of null
    at resizeLintReport (lint.js:154)
    at renderLintReport (lint.js:147)
    at update (lint.js:109)
    at cm (lint.js:50)

tophf · 2017-08-18T12:39:23Z

_locales/bg/messages.json

@@ -29,7 +29,7 @@
 	},
 	"issues": {
 		"message": "Проблеми",
-		"description": "Label for the CSSLint issues block on the style edit page"
+		"description": "Label for the stylelint issues block on the style edit page"


This one should be CSS linter in all language files to mean both CSSLint and stylelint.

tophf · 2017-08-18T12:46:12Z

_locales/bg/messages.json

@@ -665,8 +665,8 @@
 		"description": "Go to Options UI"
 	},
 	"issuesHelp": {
-		"message": "Проблеми, намерени от <a href='https://github.com/CSSLint/csslint' target='_blank'>CSSLint</a> при следните правила:",
-		"description": "Help popup message for the CSSLint issues block on the style edit page"
+		"message": "Проблеми, намерени от $link$ при следните правила:",


All messages with $variable$ should include "placeholders" object which you can copy from the English locale file by running tools/pull_locales_postprocess.py

tophf · 2017-08-18T12:58:57Z

_locales/en/messages.json

  },
  "issuesHelp": {
-    "message": "The issues found by <a href='https://github.com/CSSLint/csslint' target='_blank'>CSSLint</a> with these rules enabled:",
-    "description": "Help popup message for the CSSLint issues block on the style edit page"
+    "message": "The issues found by $link$ with these rules enabled:",


We had a bug in localization.js::tHTML() - the top-level text nodes were skipped, I've pushed a fix in master: 21b2ba5

tophf · 2017-08-18T13:01:03Z

_locales/bg/messages.json

-		"message": "Проблеми, намерени от <a href='https://github.com/CSSLint/csslint' target='_blank'>CSSLint</a> при следните правила:",
-		"description": "Help popup message for the CSSLint issues block on the style edit page"
+		"message": "Проблеми, намерени от $link$ при следните правила:",
+		"description": "Help popup message for the selected linter issues block on the style edit page"


selected CSS linter in all files to be consistent with "issues" message

tophf · 2017-08-18T13:04:54Z

edit.html

@@ -33,10 +34,15 @@

    <script src="vendor/codemirror/addon/edit/matchbrackets.js"></script>

-    <link rel="stylesheet" href="vendor/codemirror/addon/lint/lint.css" />
+    <!-- CSSLint -->


Nope, linter script and stylesheets should only load when selected in the UI or at the start in edit.js if corresponding prefs.get('editor.linter') is selected. Ditto for csslint. You can use some well-known script loader but only if it's really tiny or write your own or rework/adapt mine.

The stylesheet is part of CodeMirror's linting tooltips. I would think that should be loaded no matter which linter is selected. Also, since CSSLint is the default and small, should that be loaded by default? Or do you want it to be dynamically loaded?

Well, you can add a third option "none" to the linters selector. It would be internally represented as an empty string in prefs and <option value="" i18n-text="genericNoneLabel"> in html. And genericNoneLabel in _locales/en/messages.json.

What if I used the "genericDisabledLabel" like that used in the Highlight dropdown?

And should "None" be the default? Or keep it on CSSLint?

genericDisabledLabel is okay, I forgot about it.

ideally, stylelint should be the default, but it's too soon to make it so, we and other users need to test it, you may be able to reduce the bundle etc.

so, practically, we should preserve the historical behavior: keep csslint

tophf · 2017-08-18T13:34:00Z

edit/lint.js

+    const link = '<a target="_blank" href="https://stylelint.io/demo/">Stylelint</a>';
+    const text = JSON.stringify({rules: rules}, null, 2);
+    const content = `<textarea class="contents settings">${text}</textarea>
+      <p>${t('setStylelintLink', link)}</p>


We somewhat decided to use $element to construct DOM in new code we write due to AMO nitpicking on innerHTML. You can look for examples in the existing code, it's pretty flexible for a 20-line function.

The same applies to other places except usages of t() with localizable html.

Will $element() replace tHTML() then?

Eventually we'll replace all tHTML usage along with the function itself. Meanwhile we use it only for translated messages, and $element for the new DOM-building code.

tophf · 2017-08-18T13:36:48Z

edit/lint.js

+        }
+      }, 3000);
+    }
+    return false;


Not needed in this case AFAICT. Also, better to preventDefault() explicitly if that's what you want instead of return false which was popular in the old days of js.

tophf · 2017-08-18T13:37:15Z

edit/lint.js

+    if (prefs.get('editor.linter') === 'stylelint') {
+      updateLinter('stylelint');
+    }
+    return false;


tophf · 2017-08-18T13:43:49Z

vendor-overwrites/codemirror/addon/lint/linter.js

@@ -0,0 +1,102 @@
+// CodeMirror, copyright (c) by Marijn Haverbeke and others


Er, css-lint.js is the original name of this CodeMirror addon so it should remain the same.

tophf · 2017-08-18T13:54:28Z

vendor-overwrites/codemirror/addon/lint/stylelint-config.js

+  rules: {
+    // ** recommended rules **
+    // ref: https://github.com/stylelint/stylelint-config-recommended/blob/master/index.js
+    'at-rule-no-unknown': [ true, { 'severity': 'warning' }],


The extra spaces after/before brackets aren't needed: we all use IDE with syntax highlight, not a plain text notepad. While these embellishments seem trendy and hip the existing code doesn't use this style.

The quotes around severity aren't needed as well and only add visual clutter. Actually I would probably extract it to a variable:

const stylelintDefaultConfig = (defaultSeverity => ({ syntax: 'sugarss', rules: { 'at-rule-no-unknown': [true, defaultSeverity], 'block-no-empty': [true, defaultSeverity], ////////////////// }, }))({severity: 'warning'});

Mottie · 2017-08-18T16:16:36Z

I'll get to the rest of the changes when I get more time later today.

Mottie · 2017-08-19T18:10:12Z

@tophf Here is my attempt at using rollup to bundle stylelint... the readme includes details on why I haven't got it working yet.

okay

tophf · 2017-08-20T07:17:05Z

As for the bundle size, I thought it might be legit because the bundle includes implementation for all rules.
The config icon should not be shown for CSSLint or you need to implement the actual config routine, which is currently hardcoded in vendor-overwrites/codemirror/addon/lint/css-lint.js to 5 rules.

tophf · 2017-08-20T07:18:54Z

Ah, and please rebase on current master branch, then reupload the PR via git push -f

tophf · 2017-08-20T07:26:10Z

And remove the unminified bundle, we'll risk it.

Mottie · 2017-08-20T14:54:38Z

All done except for one remaining issue...

When the stylelint settings panel opens it used to look like it was working because the css syntax highlighting was applied. Now that I have the json-lint.js and jsonlint library loaded. The syntax highlighting and linting doesn't appear to be working. This appears to be the correct json mode code:

cm.setOption('mode', 'application/json');
cm.setOption('lint', 'json');

Can you tell if I'm missing something?

tophf · 2017-08-28T12:38:23Z

@Mottie, I've reworked the config-related things myself as it was too complicated to explain. And changed a few other trivial things. Turns out you've used 'null' when no linter is active, which I thought pretty weird (what if we add a null linter? lol), so an empty string is used (IIRC this is what I suggested initially).

Mottie · 2017-08-28T12:56:09Z

I think you're actually supposed to set the CodeMirror option to null, but the code was expecting a string in a few places and 'null' also worked, so I left it 😛.

I'm glad it's finally merged! 🎉!

tophf · 2017-08-28T13:03:01Z

It's supposed to be false and your code was setting it correctly in getForCodeMirror (in the ternary's alternative branch). I didn't change that.

tophf · 2017-08-31T04:58:25Z

BTW, an easy instruction or a build script will be required on AMO to produce an exact copy of our minified bundle.

Mottie · 2017-08-31T05:07:40Z

I included a markdown file linking to the repo...

https://github.com/openstyles/stylus/blob/master/vendor-overwrites/stylelint/stylelint-mod.md

We'll need to update it again since v8.1.0 will be released soon.

tophf · 2017-08-31T05:10:30Z

I know, but I don't see an easy instruction or a build script. I meant something that @schomery can include in a note when submitting to AMO.

Mottie · 2017-08-31T05:12:39Z

https://github.com/Mottie/stylelint/tree/mod#stylelint-mod

tophf · 2017-08-31T05:17:01Z

I know, but I'm afraid the AMO drones won't be able to follow that. They also don't have time. When I say an easy instruction I mean something that's really easy.

tophf · 2017-08-31T05:18:40Z

A build script that performs all modifications would be ideal.

Mottie · 2017-08-31T05:20:53Z

Ok, I'll see what I can do.

tophf · 2017-09-07T16:31:51Z

1.1.4.1 is listed on AMO so apparently they've managed to follow your instructions.

Mottie · 2017-09-07T17:07:36Z

Nice! Hmm, too bad the Chrome version still hasn't been approved 😞

eight04 mentioned this pull request Aug 16, 2017

Install styles from *.user.css file #134

Merged

27 tasks

tophf previously requested changes Aug 18, 2017

View reviewed changes

Mottie added 15 commits August 27, 2017 17:20

Don't show nested "rules" in config

52af5c8

Make linter config JSON more readable

95ff72b

Clean up lint globals

2df6b8b

Add config cancel button

2c23489

Sort "cm_*" messages

ef197bc

Clear linter config history

b00d957

Rename CSSLint rule config file & variable

e96f080

Refocus textbox after save/reset

1a38414

Add linter config reset tooltip

cb177eb

Do not automatically update the linter after a config reset

5ad23bd

Fix linter config tooltip

1d1a2ce

Rename & organize linter messages

1e03910

Rename CSSLint ruleset to config

21e22c2

Use rules or config consistently

6c3501b

Set selected linter during import

fdeffde

tophf merged commit da565a5 into openstyles:master Aug 28, 2017

Mottie mentioned this pull request Sep 6, 2017

Stylus catched some errors with the CSS StylishThemes/GitHub-Dark#534

Closed

Mottie deleted the stylelint branch September 7, 2017 17:07

		@@ -0,0 +1,102 @@
		// CodeMirror, copyright (c) by Marijn Haverbeke and others

Include both CSSLint and stylelint #150

Include both CSSLint and stylelint #150

Conversation

Mottie commented Aug 13, 2017

tophf commented Aug 13, 2017 • edited Loading

tophf commented Aug 13, 2017 • edited Loading

tophf commented Aug 13, 2017

narcolepticinsomniac commented Aug 13, 2017

Mottie commented Aug 13, 2017

tophf commented Aug 13, 2017 • edited Loading

tophf commented Aug 13, 2017 • edited Loading

Mottie commented Aug 13, 2017

tophf commented Aug 13, 2017

tophf commented Aug 13, 2017 • edited Loading

narcolepticinsomniac commented Aug 13, 2017 • edited Loading

tophf commented Aug 13, 2017

Mottie commented Aug 13, 2017

Mottie commented Aug 13, 2017

tophf commented Aug 13, 2017

Mottie commented Aug 15, 2017 • edited Loading

tophf commented Aug 15, 2017

tophf commented Aug 16, 2017

Mottie commented Aug 17, 2017 • edited Loading

Mottie commented Aug 17, 2017

tophf commented Aug 17, 2017

tophf 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

tophf Aug 18, 2017 • edited Loading

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

tophf Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mottie Aug 18, 2017 • edited Loading

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

tophf Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mottie commented Aug 18, 2017

Mottie commented Aug 19, 2017

tophf commented Aug 20, 2017

tophf commented Aug 20, 2017

tophf commented Aug 20, 2017

Mottie commented Aug 20, 2017 • edited Loading

tophf commented Aug 28, 2017

Mottie commented Aug 28, 2017

tophf commented Aug 28, 2017

tophf commented Aug 31, 2017

Mottie commented Aug 31, 2017 • edited Loading

tophf commented Aug 31, 2017

Mottie commented Aug 31, 2017

tophf commented Aug 31, 2017

tophf commented Aug 31, 2017

Mottie commented Aug 31, 2017

tophf commented Sep 7, 2017

Mottie commented Sep 7, 2017

tophf commented Aug 13, 2017 •

edited

Loading

tophf commented Aug 13, 2017 •

edited

Loading

tophf commented Aug 13, 2017 •

edited

Loading

tophf commented Aug 13, 2017 •

edited

Loading

tophf commented Aug 13, 2017 •

edited

Loading

narcolepticinsomniac commented Aug 13, 2017 •

edited

Loading

Mottie commented Aug 15, 2017 •

edited

Loading

Mottie commented Aug 17, 2017 •

edited

Loading

tophf Aug 18, 2017 •

edited

Loading

tophf Aug 18, 2017 •

edited

Loading

Mottie Aug 18, 2017 •

edited

Loading

tophf Aug 18, 2017 •

edited

Loading

Mottie commented Aug 20, 2017 •

edited

Loading

Mottie commented Aug 31, 2017 •

edited

Loading