Add eslint and update noVNC to ES6 #1013

juanjoDiaz · 2018-02-01T16:00:55Z

New attempt to move the library to ES6.

I'd start pushing more simple commits so it's simple to review.

juanjoDiaz · 2018-02-01T17:52:50Z

Ok. This is good now. I hope that the smaller and more focused commits help reviewing the changes.

This currently uses eslint:recommended config. That's the loosest configuration and doesn't really enforce anything (for example, it doesn't even enforce using let/const instead of var). So the next step is to put it a more strict eslint config and fix the code accordingly.

I'd suggest airbnb config since it's widely used and imho it's rock solid. Wdyt??

juanjoDiaz · 2018-02-04T17:17:27Z

Hei guys,

Any ETA of when this could be merge?
I have a couple other PRs coming after this one :)

CendioOssman · 2018-02-05T12:09:57Z

I'd suggest airbnb config since it's widely used and imho it's rock solid. Wdyt??

Do a separate commit with that config applied and we can see what it will do to the code.

Any ETA of when this could be merge?
I have a couple other PRs coming after this one :)

That would be after 1.0 is out. So unfortunately you got hit with some bad timing here. :/

DirectXMan12

lots of comments inline. Some good work, some things that I don't think we should do, some stuff that's not quite correct.

In general, ease back on the use of the ternary operator, since, IMO, it generally makes things a bit harder to read for complicated expressions.

(please also look at the "outdated" comments, since some are still relevant for a particular commit)

DirectXMan12 · 2018-02-07T14:52:32Z

app/localization.js

@@ -48,7 +48,8 @@ Localizer.prototype = {
            }

            // First pass: perfect match
-            for (var j = 0;j < supportedLanguages.length;j++) {
+            var j;


I think this just makes the code harder to read. f we're going to transpile, these should just be let declarations.

nvm, you do it in the next commit

Yeah. I tried to keep every commit as small as possible.

DirectXMan12 · 2018-02-07T14:55:22Z

core/rfb.js

@@ -1062,7 +1060,7 @@ RFB.prototype = {
        var serverSupportedTypes = [];

        for (var i = 0; i < subAuthCount; i++) {
-            var capNum = this._sock.rQshift32();
+            this._sock.rQshift32();


this has lost its self-documenting properties. Please add a comment indicating what the shift does instead.

Same as above. I added the comment in the next commit.

DirectXMan12 · 2018-02-07T14:56:45Z

core/util/logging.js

@@ -24,8 +24,10 @@ export function init_logging (level) {
        _log_level = level;
    }

-    Debug = Info = Warn = Error = function (msg) {};
+    Debug = Info = Warn = Error = function () {};


Shouldn't these have a var declaration somewhere?

They do. In lines 15-18

DirectXMan12 · 2018-02-07T14:57:55Z

package.json

@@ -7,7 +7,8 @@
    "test": "tests"
  },
  "scripts": {
-    "test": "PATH=$PATH:node_modules/karma/bin karma start karma.conf.js",
+    "lint": "eslint app core tests utils",
+    "test": "karma start karma.conf.js",


why have you dropped the extra PATH setting. It used to be necessary if node_modules wasn't on your path.

scripts in NPM automatically look in the node_modules folder.

DirectXMan12 · 2018-02-07T15:03:09Z

tests/test.keyboard.js

+                var evt1 = keyevent('keydown', {code: 'KeyA', key: 'a'});
+                kbd._handleKeyDown(evt1);
+                expect(evt1.preventDefault).to.have.been.called;
+                var evt2 = keyevent('keyup', {code: 'KeyA', key: 'a'});


why is this necessary? We can re-use variables...

We can. This change was looking forward to the next commit and the javascript best practice of using immutable variables (const) whenever possible.

DirectXMan12 · 2018-02-07T16:09:36Z

core/rfb.js


-            return this._fail('Security negotiation failed' + context);
-        }
+        return this._fail('Security negotiation failed' + context +


template literal

DirectXMan12 · 2018-02-07T16:11:24Z

app/ui.js

-import Keyboard from '../core/input/keyboard.js';
-import RFB from '../core/rfb.js';
-import * as WebUtil from './webutil.js';
+import * as Log from '../core/util/logging';


These need to stay in. Support for bare names is unclear in the WHATWG spec, and the module loader polyfill doesn't permit it: https://github.com/ModuleLoader/browser-es-module-loader

I can bring it back.

It's weird though, that it hadn't fail any of the tests...

DirectXMan12 · 2018-02-07T16:14:43Z

app/localization.js

@@ -31,7 +31,7 @@ export class Localizer {
            ? window.navigator.languages
            : [navigator.language || navigator.userLanguage];

-        for (let i = 0; i < userLanguages.length; i++) {
+        for (let i = 0; i < userLanguages.length; i += 1) {


just because Crockford is afraid of the increment operator doesn't mean we should be too. Please leave these in.

Sure.

I found ++i, i++ and i +=1 all over the code. that's why I tried to normalize with one.

++i vs i++ was making not so clear to me what the code was doing in some cases so I preferred i += 1.

In the case of for loops I agree that's irrelevant. But you can't tell eslint to enforce it everywhere except for for loops...

DirectXMan12 · 2018-02-07T16:16:04Z

core/util/logging.js

@@ -17,7 +17,7 @@ let Info = () => {};
 let Warn = () => {};
 let Error = () => {};

-export function init_logging (level) {
+export function init_logging(level) {


this doesn't look normalized -- some have spaces between parens and the name, some don't. I prefer spaces.

DirectXMan12 · 2018-02-07T16:17:02Z

app/localization.js

@@ -152,5 +152,5 @@ export class Localizer {
    }
 }

-export let l10n = new Localizer();
+export const l10n = new Localizer();


squash all these into the previous let/const/var commit, if possible

This is the let/const/var commit, or am I missing something?

samhed

I agree with the comments by @DirectXMan12 and added some more. Did you test this?

samhed · 2018-02-08T08:53:40Z

core/display.js

@@ -657,7 +657,7 @@ export default class Display {
            return;
        }

-        const cur = []
+        const cur = [];


The commit-message should be "Add missing semicolons"

samhed · 2018-02-08T09:31:47Z

core/rfb.js

@@ -718,6 +715,7 @@ RFB.prototype = {
                Log.Error("Got data while disconnected");
                break;
            case 'connected':
+                // eslint-disable-next-line no-constant-condition


Meh, I don't like that we need to have this comment here.. Is it just to disable an eslint warning?

Yup. We could disable it globally. However, I think that's reasonable to assume that a condition on a constant should be an exception and not the norm. So it's disabled globaly and enabled only for this line.

I don't like that we need to disable it in this case though. What is the eslint recommended way for an infinite loop?

@juanjoDiaz, did you check if there was a better way that would make lint happy?

there's apparently a checkLoops setting for exactly this. So I suggest turning on that option.

Endless loops are dangerous. That's why there shouldn't be used unless strictly necessary.
That's why the no-constant-condition and checkLoops rule is enable by default. Disabling it will not indicate endless loop anywhere in the code.
That's why I think that using a comment is good to indicate that this particular endless loop is an exception and not the norm.

If you still disagree and prefer you to completely disable the rule. Let me know and I can do it.

My vote is to disable this rule since I consider this particular pattern to be a common and useful one.

What does everyone else think?

Agreed, +1 for disabling the rule

samhed · 2018-02-08T09:33:31Z

core/util/logging.js

-var Debug = function (msg) {};
-var Info = function (msg) {};
-var Warn = function (msg) {};
-var Error = function (msg) {};


Aren't these function arguments required?

Nope. Arguments are never required in javascript. And those are never used anyway. It's just a default no-op

samhed · 2018-02-08T09:35:01Z

core/util/polyfill.js

@@ -10,7 +10,7 @@
 if (typeof Object.assign != 'function') {
    // Must be writable: true, enumerable: false, configurable: true
    Object.defineProperty(Object, "assign", {
-        value: function assign(target, varArgs) { // .length of function is 2


this snippet was taken directly from MDN I think, perhaps we should keep it as it was?

It's an argument that's not used.

To add it and not have eslint failing we would need to either add a comment to disable the rule here or disable eslint checks for the entire file (which might be trouble if we add here other polyfills.)

ok, it's fine then

samhed · 2018-02-08T09:41:10Z

app/localization.js

-        }
+        const userLanguages = (typeof window.navigator.languages === 'object')
+            ? window.navigator.languages
+            : [navigator.language || navigator.userLanguage];


Code was a lot easier to read before this change

I see that you all really dislike ternary operators.

We differ on that. But I can keep the if-else blocks.

As mentioned above, the main advantage of if-else is to declare variable as const when they are inmutable after declaration.

In my mind they are fine if the condition and results are simple and you can keep it short.

samhed · 2018-02-08T10:02:20Z

app/ui.js

        l10n.dictionary = translations;

        // wait for translations to load before loading the UI
        UI.prime();
-    }, function (err) {
+    }, (err) => {


Should be able to skip the '{' and '}' here right?

yup. But this change was just changed on master

samhed · 2018-02-08T10:14:22Z

core/rfb.js

-        } else {
-            context = ' on ' + context;
-        }
+        context = typeof context === 'undefined'


samhed · 2018-02-08T10:15:13Z

core/rfb.js

+                ? { status: security_result_status }
+                : { status: security_result_status, reason }
+            });
+        this.dispatchEvent(event);


in other places you have removed the intermediate event variable in cases like these

samhed · 2018-02-08T10:16:54Z

app/localization.js

@@ -31,7 +31,7 @@ export class Localizer {
            ? window.navigator.languages
            : [navigator.language || navigator.userLanguage];

-        for (let i = 0; i < userLanguages.length; i++) {
+        for (let i = 0; i < userLanguages.length; i += 1) {


samhed · 2018-02-08T10:18:42Z

core/util/strings.js

@@ -9,6 +9,6 @@
 /*
 * Decode from UTF-8
 */
-export function decodeUTF8 (utf8string) {
+export function decodeUTF8(utf8string) {


this commit doesn't seem to bring consistency at all!

Anonymous function
function () {....} // One single space between function and opening parenthesis and between closing parenthesis and opening curly brace

Named function
function funcName() {....} // No space between function name and opening parenthesis and single space between closing parenthesis and opening curly brace

It's the airbnb recommended way. I can follow some other way if you would prefer that.

juanjoDiaz · 2018-02-08T18:07:17Z

Thanks for the thorough review.

I think that our main point of disagreement is the ternary operator. I can live with that :)

I'll try to fix all the comments.

juanjoDiaz · 2018-05-07T20:39:21Z

After rebasing, something weird seems to be going on with Edge. Any idea?

CendioOssman · 2018-05-09T09:01:02Z

I think it's just sauce labs that are flaky as hell.

CendioOssman

Lots of minor things. But one big thing: transpiling of classes isn't working.

CendioOssman · 2018-05-09T09:06:17Z

.eslintrc

-      "no-unused-vars": ["error", { "vars": "all", "args": "none", "ignoreRestSiblings": true }],
-      "no-constant-condition": ["error", { "checkLoops": false }],
-      "no-var": "error"
+    "no-unused-vars": ["error", { "vars": "all", "args": "none", "ignoreRestSiblings": true }],


Please move the reindent to the original commit.

The whole file is indented to 2 spaces except for this 3 lines.

I'll re-indent the whole file to 4 spaces

CendioOssman · 2018-05-09T09:07:51Z

.eslintrc

+    "no-useless-constructor": "error",
+    "constructor-super": "error",
+    "no-this-before-super": "error",
+    "no-dupe-class-members": "error"


A bunch of these are already in recommended, so why list them here?

CendioOssman · 2018-05-09T09:08:27Z

app/localization.js

        function process(elem, enabled) {
            function isAnyOf(searchElement, items) {
                return items.indexOf(searchElement) !== -1;
            }

            function translateAttribute(elem, attr) {
-                const str = self.get(elem.getAttribute(attr));
+                const str = this.get(elem.getAttribute(attr));


This looks broken.

Why?

It was doing const self = this; at the beginning of the function but we are not changing the bindings or calling the function in any scope so this will never change.

CendioOssman · 2018-05-09T09:08:39Z

app/localization.js

                elem.setAttribute(attr, str);
            }

            function translateTextNode(node) {
-                const str = self.get(node.data.trim());
+                const str = this.get(node.data.trim());


CendioOssman · 2018-05-09T09:10:55Z

app/ui.js

@@ -101,12 +101,10 @@ const UI = {

        document.documentElement.classList.remove("noVNC_loading");

-        let autoconnect = WebUtil.getConfigVar('autoconnect', false);
+        const autoconnect = WebUtil.getConfigVar('autoconnect', false);


Unrelated. Remove.

CendioOssman · 2018-05-09T10:27:25Z

tests/test.rfb.js

+                client._framebufferUpdate = function () {
+                    this._sock.rQskip8();
+                    return true;
+                };  // we magically have enough data


CendioOssman · 2018-05-09T10:27:51Z

utils/genkeysymdef.js

@@ -108,7 +108,7 @@ out +=
 "};\n" +
 "\n" +
 "export default {\n" +
-"    lookup : function(u) {\n" +
+"    lookup(u) {\n" +


Why unrelated? The whole point of the commit it to get rid of inline functions in favour of arrow function and object/class functions.

If I change keysymdef.js but not the generator, then the next time that you generate the file, the change will be reverted.

CendioOssman · 2018-05-09T10:30:28Z

utils/use_require.js

        const args = Array.prototype.slice.call(arguments);
        return new Promise((resolve, reject) => {
-            original.apply(obj, args.concat((err, value) => {
+            original.apply(this, args.concat((err, value) => {


This does not have the same behaviour as the previous code. I don't think you can use an arrow function here.

Why not?

As in the other comment above, const obj = this; and we are not doing any binding or calling the function on any other object so obj will always be equals to this.

Also, what do you mean about not being able to use an arrow function?

CendioOssman · 2018-05-09T10:31:13Z

utils/use_require.js

    return readdir(base_path)
-    .then(files => {
+    .then((files) => {


Not needed, right?

It's the convention that parenthesis are skip on 1-lines but used when using braces.

myArg => myFunc(myArgs)

vs

(myArg) => { return myFunc(myArgs); }

Would you prefer something else?

CendioOssman · 2018-05-09T10:31:54Z

utils/use_require.js

-    if (!with_app_dir) {
-        only_legacy = true;
-    }
+    const only_legacy = !with_app_dir;


This is not the same thing.

juanjoDiaz · 2018-05-10T10:50:32Z

I've fixed that issues that you commented and were clear issues and I've commented on the ones that I think that is correct.

Let's agree on what to do about the latter and try to get this merged. :)

I won't be doing any more changes as part of this PR. If I decide to do anything else (ES6 related or not) I'll do it as separate smaller PRs.

juanjoDiaz · 2018-05-10T10:52:29Z

@CendioOssman can you clarify why " transpiling of classes isn't working"??

I run npm run prepare and the transpiled code in the lib folder doesn't contain classes at all.

Edit: I didn't notice that I was supposed to commit and push the vendor/dist folder. Done now :)

CendioOssman · 2018-05-18T10:47:12Z

Unfortunately github's system are crashing when trying to load this PR. I've filed a support request, but I'm afraid we cannot continue until they've resolved the issue. :/

…

-- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköping https://facebook.com/ThinLinc Phone: +46-13-214600 https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

juanjoDiaz · 2018-05-19T20:57:19Z

@CendioOssman Would it be a solution to revert this to the const/let commit that we were all happy with, get that merged and then submit I can submit a separate PR for classes/arrow functions?

Otherwise, this might take forever, more conflicts will appear, etc.

samhed · 2018-05-21T12:08:39Z

Den Sat, 19 May 2018 13:57:22 -0700 skrev Re: [novnc/noVNC] Add eslint and update noVNC to ES6 (#1013):

@CendioOssman Would it be a solution to revert this to the `const/let` commit that we were all happy with, get that merged and then submit I can submit a separate PR for `classes/arrow functions`? Otherwise, this might take forever, more conflicts will appear, etc.

Sounds good to me.

…

-- Samuel Mannehed Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköping https://facebook.com/ThinLinc Phone: +46-13-214600 https://plus.google.com/+CendioThinLinc

juanjoDiaz · 2018-05-23T21:30:36Z

Ok. Now we just have the 3 commits that were already approved. Let's get those in and I'll do a separate PR for classes and arrow functions.

Btw, I can access the PR just fine and I don't think that we are too far from each other... No clue of what might be the problem that you are having...

juanjoDiaz · 2018-05-24T05:14:44Z

The fail test is Sauce lab being flaky as usual. the changes are exactly the same as have been pushed a million times before and passed :)

samhed

I had another look. There were still a few unaddressed comments with some changes unrelated to the themes of the commits. But since we're having such problems viewing this PR and we have been through the code multiple times I think that it looks good enough.

juanjoDiaz changed the title ~~Add eslint and fix reported issues~~ Add eslint and update noVNC to ES6 Feb 1, 2018

juanjoDiaz force-pushed the es6_refactor_2 branch 4 times, most recently from a71503d to ff3c10c Compare February 1, 2018 17:20

This was referenced Feb 1, 2018

Add eslint and refactor to ES6 #1005

Closed

websock.js out of sync with upstream #812

Open

CendioOssman self-requested a review February 5, 2018 12:10

juanjoDiaz force-pushed the es6_refactor_2 branch 3 times, most recently from 41e01af to 729eca6 Compare February 6, 2018 04:37

DirectXMan12 requested changes Feb 7, 2018

View reviewed changes

samhed requested changes Feb 8, 2018

View reviewed changes

juanjoDiaz force-pushed the es6_refactor_2 branch 13 times, most recently from e953f3a to 92e2ff4 Compare February 9, 2018 23:12

juanjoDiaz force-pushed the es6_refactor_2 branch 2 times, most recently from b162a4b to cdfbad9 Compare May 7, 2018 20:29

juanjoDiaz force-pushed the es6_refactor_2 branch 4 times, most recently from de3ba2e to 5a2c7ef Compare May 8, 2018 20:49

CendioOssman requested changes May 9, 2018

View reviewed changes

juanjoDiaz force-pushed the es6_refactor_2 branch 4 times, most recently from 5836d70 to fbcbd8b Compare May 10, 2018 10:43

juanjoDiaz force-pushed the es6_refactor_2 branch from 1a4a115 to a551039 Compare May 10, 2018 11:16

juanjoDiaz added 3 commits May 24, 2018 00:25

Add eslint and fix reported issues

8727f59

Add transpilation for IE11 and skip linux tests

cdb860a

Prefer const/let over var

2b5f94f

juanjoDiaz force-pushed the es6_refactor_2 branch from a551039 to 2b5f94f Compare May 23, 2018 21:27

samhed approved these changes May 25, 2018

View reviewed changes

samhed merged commit fe70a1d into novnc:master May 25, 2018

samhed added the feature label May 25, 2018

samhed added this to the v1.1.0 milestone May 25, 2018

juanjoDiaz mentioned this pull request May 25, 2018

Use es6 classes and arrow functs #1083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eslint and update noVNC to ES6 #1013

Add eslint and update noVNC to ES6 #1013

Add eslint and update noVNC to ES6 #1013

Add eslint and update noVNC to ES6 #1013

Conversation

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

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

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

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