[Suggestion] Started the refactoring of the janus.js

[oscarvadillog]

I admire this project and all the work and effort behind it, in fact it's a key component of my current project.

I would like to participate in the refactoring of the janus.js library. In my opinion, it can be improved a lot in terms of good practices of JavaScript (ES5) without affecting the behavior with the Janus Server.

This is a first commit where I've shown my idea. If you find it interesting let me know and I'll continue refactoring and advancing in this pull request. 😆

[lminiero]

Before even looking at this, I see a lot of changes to code style that are not needed at all, and make it pretty much impossible to just focus on the changes: e.g., a ton of if( turned to if (. Our code style is the former, so please don't change that and others.

[oscarvadillog]

I only tried to align and use always the same code style. The current code has lines with if( and others with if (. Example.. You also use double and single quotes at the same time.

On the other hand, I think there are many conditions that could be simplified, for example:
if(plugin === undefined || plugin === null) { turn to if (!plugin)
or if(apisecret !== null && apisecret !== undefined) turn to if (apisecret)

Of course I respect your style guide, my intention was only to align the code and always use the same rules. I personally like Airbnb guide style, but I understand you have yours.

In any case, this PR was just a suggestion, supposed to be unnecessary. We can close it if you want 😄

[lminiero]

I'm ok keeping it open, and I appreciate any attempt to improve the code, so thanks for that! But I won't look at it if more than half the changes have nothing to do with the patch, because I honestly don't have the time to discriminate actual improvements from visual things. If you update the patch to just keep the meaningful changes, happy to review.

[oscarvadillog]

What about changes like this? 4ba444a

[lminiero]

Thanks, much more readable and easier to follow this way!

[oscarvadillog]

WIP 👍

[oscarvadillog]

It may be enough for now. There are many lines already modified.
All demos tested in Chrome and FF and I didn't detect anything broken.

[lminiero]

@oscarvadillog thanks! Apologies for the late reply, we've been busy with a few conferences. @atoppi @GaijinKa can you have a look?

[lminiero]

Waiting for my (way more JS-savvy than I am) colleagues to look at the code, I gave a quick glance to see if I could spot anything strange, and added some notes. It's not an exhaustive review (I'll do another round in a later moment), and some notes may actually refer to similar occurrences in different parts of the code.

[atoppi]

Except for the comments I posted, it looks good to me.
Thanks @oscarvadillog for the effort.

[mtltechtemp]

Why not take the opportunity to make it an ES6 class?

In addition to having a more readable code many people would be more inclined to contribute to the code because many stop when they see ES5 class.

Edit: And for backwards compatibility you just have to provide a version transpiled by Babel.

There's also Lebab which could help to perform safe transform on the code. (It does exactly the opposite of what Babel does)

[lminiero]

I would personally object to anything that sensibly transforms the code, since again, I'm the one supposed to maintain it, and I have no intention of doing so with a much more complex JS code I'd likely not understand. Besides, I don't want any dependency in there (if that's what you meant by Babel).

[mtltechtemp]

I would personally object to anything that sensibly transforms the code, since again, I'm the one supposed to maintain it, and I have no intention of doing so with a much more complex JS code I'd likely not understand. Besides, I don't want any dependency in there (if that's what you meant by Babel).

I think you're wrong. ES6 looks like C, C#.
I believe it be easier for you since you're a C guy.

Edit: Babel is not a dependency. It is a code transform tool.

[lminiero]

Be that as it may, I won't accept any major change to janus.js until the unified-plan branch is merged. It would generate a ton of conflicts with the changes we have there, and I have to fight with those enough as it is.

[mtltechtemp]

Be that as it may, I won't accept any major change to janus.js until the unified-plan branch is merged. It would generate a ton of conflicts with the changes we have there, and I have to fight with those enough as it is.

It's not about to be fussioned since it's been sleeping since December 8, 2018. (Except for bugfixes)
Or am I wrong?

[lminiero]

I still want it to be merged, and it will be merged. I wanted to avoid doing that until enough people test it and tell me it's ok, as I wanted to be sure it wouldn't break too many things. Unfortunately that hasn't happened, so one day I'll just do it.

[mtltechtemp]

I still want it to be merged, and it will be merged. I wanted to avoid doing that until enough people test it and tell me it's ok, as I wanted to be sure it wouldn't break too many things. Unfortunately that hasn't happened, so one day I'll just do it.

Well, you did your job, you warned and asked for feedback.
It's been almost a year, I think it's time.

[lminiero]

Well, you did your job, you warned and asked for feedback.
It's been almost a year, I think it's time.

That's what I wanted to hear! 😄
Jokes apart, I'll probably send a last reminder after the IETF (last week of November), to then merge as the first thing on 2020. The plan is to keep the latest 0.x version as a separate branch anyway, rather than a tag as we did so far with versions: while we won't develop new features or accept pull requests on that, it would still allow us to push fixes when needed while people transition to the new version.

[oscarvadillog]

Thanks for all your comments @atoppi

What do we do with the for ... in loops? Right now there are 11 loops in the janus.js file

[atoppi]

Maybe @lminiero has some comments about this but if you ask me I'd switch the for ... in loops on the arrays with the appropriate for ... of constructs.

[oscarvadillog]

Ok, we wait for @lminiero
If you agree I can make that change in all the loops.

[lminiero]

Sure: as far as I've understood it should be a simple change in the way the for loop is invoked, and so nothing too disruptive. Thanks again for taking care of all this, it's hugely appreciated!

[oscarvadillog]

Already done

[atoppi]

Hi @oscarvadillog,
there two issues:

1. When using the for ... of loop you could already get the element of the array.

This means that the old

for(var index in config.pc.getSenders()) {
  // get "s"
  var s = config.pc.getSenders()[index];

become

for(var s of config.pc.getSenders()) {
  // already got "s"

But you are still reading the element

for(var index of config.pc.getSenders()) {
  var s = config.pc.getSenders()[index];

and this is breaking the code.

2. You have to change the construct only for the Arrays, not also for the Object.
   For example Janus.sessions is an Object, so you need to keep the for ... in (unless you are not inspecting an iterable object, but this is not our case).

[oscarvadillog]

That's right! I'll check back later.

[oscarvadillog]

Already done, your review @atoppi please

[lminiero]

He's currently abroad for a conference, so he'll probably only be able to review when he's back, towards the end of the week. From a quick glance, it looks fine to me, but I don't trust myself much when it comes to JavaScript 🙂

[lminiero]

While we wait for the green light from @atoppi, I had another look at the changes and added a few more notes. Apart from that, this is good to merge for me 👍 (after I test it, of course, which I haven't done yet)

[oscarvadillog]

Resolved. Thx @lminiero 😄

[oscarvadillog]

Waiting for the green light from @atoppi ❤️

[atoppi]

Sorry guys for the late reply, LGTM 👍

[lminiero]

Merging then, thanks @oscarvadillog for the great work!