Javascript code needs reviewed

[raidancampbell]

I've never done this Javascript stuff before, and I'm absolutely certain there are fundamental practices that I'm disobeying. It would be awesome if someone more experienced in Javascript could take a look at the code.

Javascript code is located in the static/js directory, and my main concern is the index.js file

[dead]

I'm not really a front-end dev but I will try to help 😄
I like when the code is structured like the 'NvHTTP' And I would try to accesing html only in function like addApp(app) or get/setFramerate

[Aghassi]

I'll have a look. I've worked in frontend development.

[Aghassi]

@raidancampbell @cgutman What are your thoughts on moving to EmberJS as a framework to work around? This way we have some standards to follow instead of just doing pure JS (and avoid using JQuery directly).

[raidancampbell]

I have no preferences on the matter. I'm slightly weary of bringing in an additional framework. Would this completely replace JQuery?

@dead do you have any thoughts on the matter?

[Aghassi]

@raidancampbell Most JS frameworks now-a-days are built on top of JQuery, though I would have to check about Ember. I'm just looking to use something a little more structured is all.

[dead]

I like the idea of using Ember 👍 Just be careful in removing JQuery because NvHTTP uses it (https://github.com/moonlight-stream/moonlight-chrome/blob/master/static/js/utils.js#L204)

[raidancampbell]

sounds good to me, then

[dead]

I was reviewing the index.js file but my windows went full retard and I lost the file 😢

The thing that most annoy me is addHost cancelAddHost continueAddHost. Doesn't make sense break the addHost in 3 functions. Here some example of what can be done:

function onAddHostClick() {
    var modal = //

    modal.show_modal();

    modal_button_close.on('click', function() {
        modal.close();
    });

    modal_button_ok.on('click', function () {
        // do stuff here
        modal.close();
    });
}

In the addHostToGrid instead of using a global function in removalButton.click(confirmUnpairHost); I thing something like this works better because you can pass the host to the onRemoveHostClick function.

removalButton.click(function () {
      onRemoveHostClick(host);
});

cell.click(hostChosen); to:

cell.click(function () {
      onHostClick(host);
});

The api variable is used by more than one address? The NvHTTP is a "class" and each host should have its own instance of NvHTTP. The global api should be only used by the current paired/showing apps host. It should even be renamed to something better.

And the pooling doesn't make any sense to me. Every server has a timer to update itself? The hosts should be updated by only one global timer. And the apps should be only updated if you are in the 'showAppsMode'.

About Ember or other frameworks, I don't think they are really needed for this project. They only will increase the dependency to compile and develop.

[raidancampbell]

I fully agree with your points that addHost cancelAddHost continueAddHost, it honestly never occurred to structure it like that.

I took a half-assed stab at removing the global api variable a while ago, and need to finish the job and just pass it through. Additionally, I've mangled the original api into the concept of a full server, so some refactoring is in order.

The polling was again an implementation choice: it never occurred to me to have one timer firing for a globally shared list of hosts. This is something I'll look into.

Again, thanks for your help. Your javascript insight is very valuable to me.

[raidancampbell]

@dead now I'm remembering why I didn't structure the addHost cancelAddHost continueAddHost the way you recommended. The button onClick actions for the generated modal will get duplicate event handlers.

so let's use the exact version I wrote as a sample:

function addHost() {
    var modal = document.querySelector('#addHostDialog');
    modal.showModal();

    // drop the dialog if they cancel
    $('#cancelAddHost').on('click', function() {
        modal.close();
    });

    // try to pair if they continue
    $('#continueAddHost').on('click', function () {
        var inputHost = $('#dialogInputHost').val();
        var _nvhttpHost = new NvHTTP(inputHost, myUniqueid, inputHost);

        pairTo(_nvhttpHost, function() {
                beginBackgroundPollingOfHost(_nvhttpHost);
                addHostToGrid(_nvhttpHost);
                saveHosts();
            }, function() {
                snackbarLog('pairing to ' + inputHost + ' failed!');
        });
        modal.close();
    });
}

if I click the addHost button (and therefore run this function), then hit the cancel button, each of the click event listeners is still added. After repeatedly clicking and canceling, I have several duplicate functions that all try to run after clicking the button.

With the global event listener attachment, I avoided this problem. Do you have a trick to avoid the problem here?

[dead]

I don't think it's different when using the global handler. For each time you click in addHost you will have only one function that will be called when clicking in the cancel or continue button.

[raidancampbell]

Perhaps "global" is the wrong word. when I defined $('#continueAddHost').on('click', function () {...}); in the attachListeners, there was no problem. The attachListeners function ran once, so the function was attached once.

In this new addHost function, $('#continueAddHost').on('click', function () {...}); gets executed every time I run the addHost button, which means it gets run every time I click the add host button. Therefore, after clicking the button n times, $('#continueAddHost').on('click', function () {...}); is executed n times. When I click the button for the n+1th time, the attached function (the {...}) is executed n+1 times.

I can push this code to a branch if you don't understand what I mean.

[dead]

Oh, now I understand it! I was thinking that .on('click') would replace the older event handler, but it does not!
But I found a solution, just use off() before and it will release the older ones. see: https://jsfiddle.net/9azarL5s/3/

[dead]

I reviewed most of the index.js code, you can check it in my fork:
https://github.com/dead/moonlight-chrome/
Can you help me test it and review it? Hope you like it 😄

The changes are:

• Renamed a lot stuff
• Tried to standardize some stuff
• Removed duplicate code.
• Removed the pooling system from NvHTTP
• Removed the app box cache from NvHTTP

I think the code now is simpler and easier to understand.

[raidancampbell]

Have you tested this from scratch? I was unable to pair with your fork.

Here's a paste of the JS log, I haven't had time to actually investigate this. The hostname I was pairing with is replaced with [raidancampbell's server hostname]

[dead]

It's working fine locally, didn't test with a remote host. And I don't see the snackbar logs in your file, it should be there (https://github.com/dead/moonlight-chrome/blob/master/static/js/index.js#L791).
Fixed and improved some stuff, can you try it again?

[jorys-paulin]

Should we close this issue now that it is implemented?