search

minetest / serverlist

The global Minetest server list server
GNU Lesser General Public License v2.1
51 stars 28 forks source link

Slim down dependencies (node.js / doT.js) #21

Closed nOOb3167 closed 6 years ago

nOOb3167 commented 6 years ago

The tool install and template compilation steps can now be skipped.

The master server list functionality is not touched by these changes, just the web browser page frontend (The table displayed when navigating a web browser to http://servers.minetest.net/).

I've tested locally with one server. If any regressions remain I'll investigate.

sfan5 commented 6 years ago

The point of a template is that it's easier to edit than some JS code :-1:

Other than that your JS code

nOOb3167 commented 6 years ago

Template ease of use

The template was edited two times in history of the project: 5de6082f5770b9c9ff389fa8aa306eba911fca34 (Design changes on the server list page) bbd3c127f1df6701945d54102e22208bf932c25f (Tweak server list style)

Out of which, the second commit dates to 2015-01-10.

Here are two examples from 5de60 changeset, and what they would become after this PR:

Before: {{=constantWidth(server.clients + '/' + server.clients_max, 3.4)}} {{? !master.no_avgtop}} {{=constantWidth(Math.floor(server.pop_v) + '/' + server.clients_top, 3.4)}}{{?}}

After: constantWidth(server.clients + "/" + server.clients_max, 3.4) + constantWidth(Math.floor(server.pop_v) + "/" + server.clients_top, 3.4) +

Before: {{=constantWidth(humanTime(server.uptime), 3.2)}} / {{=constantWidth(humanTime(server.game_time), 3.2)}}

After: td.innerHTML = constantWidth(humanTime(server.uptime), 3.2) + "/" + constantWidth(humanTime(server.game_time), 3.2)

Tweaks to the template are done every few years and will look about the same after this PR.

Next, chrome has a built-in Javascript debugger (access through 'Inspect' entry of right click menu).

c0

This can be used to step through the code and actually inspect and see the elements as they are being constructed:

c1

And here is the partially constructed DOM tree at that point:

c2

The Javascript function first sets up the 'frame' with the title, <\table><\thead><\tbody> elements, then iterates over the servers list to fill the body up with <\tr> and <\td> elements.

During the iteration process you can literally see as the elements fill up:

c3

The constructs used in javascript are not that dissimilar to those in the template:

Template: <\td class="ping"> {{=constantWidth(Math.floor(server.ping 1000), 1.8)}}{{? server.lag}} / {{=constantWidth(Math.floor(server.lag 1000), 1.8)}}{{?}} </\td>{{?}}

Javascript: var td = document.createElement("td"); td.className = "ping"; td.innerHTML = constantWidth(Math.floor(server.ping 1000), 1.8) + "/" + constantWidth(Math.floor(server.lag 1000), 1.8); tr.appendChild(td);

It is possible to recognize and match-up the individual concepts: <\td> to document.createElement("td") class="ping" to td.className = "ping" text inside <\td> to td.innerHTML = blahblah

The Javascript code flow corresponds to how the HTML output is laid out closely enough that it is possible to reasonably find code generating a specific element.

Here is setting a breakpoint when an element is changed by Javascript:

c4

The Javascript is debuggable and instrumentable.

The template on the other hand it either works or not - template is compiled to a blob. Documentation (http://olado.github.io/doT/index.html) is just not that good. https://github.com/olado/doT/blob/master/doT.js check out the source code, parser is cobbled up out of regex, with corresponding quality of error diagnostics. And it requires knowing doT.

The template system is not even easier to debug and maintain and gets changed less than once a year 10 or less lines at a time. Do we really need to pull node.js and other deps, then have a template pre-compilation step to deploy the server?

does not respect any of the master.no_{total,address,clients,..} settings

The master.no_ settings are meant to be set where list.js is included (ex index.html) and have not once been set or changed within the repository. Therefore the settings likely never had a real purpose. It is good time to use the four year of experience and cut the settings code accordingly.

The only other consumer (main is http://servers.minetest.net/) of server list functionality, as far as I can tell is https://www.minetest.net/servers/ .

However it is broken (Both Firefox and Chrome). Literally the server list does not even load:

c5

c6

It does not seem settings are actually in use. Further, bitrot / technical debt have acumulated and some parts of the master server, as seen above, do not even work. Instead of mechanically seeking parity with existing / previous code, maybe it is time for some rearchitecting. Is this functionality actually used or needed, because it does not seem so.

is missing a clickable More... link when the displayed list is limited

The above goes for the "More..." link, too. The whole server list is fetched just not displayed . So server load is the same regardless of limit. Therefore need for this functionality must arise at the client. To ever have a sensible use case for "More...", there needs to be a situation where it is not OK to display the entire list, and therefore a limit is applied. But if it is not OK to display the entire list, how comes it is OK to have a "More..." button doing exactly that?

makes too many assumptions about the server entries (any of server.{mods,mapgen,clients_list,...} might not be available)

You will notice that assumption guards like below, in the template: {{=server.creative ? 'Cre ' : ''}} {{=server.damage ? 'Dmg ' : ''}} {{=server.pvp ? 'PvP ' : ''}}

Have equvalent in Javascript: (server.creative ? "Cre " : "") + (server.damage ? "Dmg " : "") + (server.pvp ? "Pvp " : "") +

The {{= server.blah }} template constructs do no guarding or magic, they make the same assumption as using server.blah directly.

What is the actual functionality that is missing?

As far as I can tell better debugability and fewer obscure web framework required.

Since server list functionality is not touched, just the web table display, maybe it is worthwhile to take a risk, then go through a few design revisions if defects arise. In worst case scenario we have broken web table display (it is already broken at https://www.minetest.net/servers/ , as screenshotted above).

sfan5 commented 6 years ago

The template was edited two times in history of the project:

And? Maintenance is about the future, not about how often stuff was touched in the past.

Here are two examples from 5de60 changeset, and what they would become after this PR: Before: {{=constantWidth(server.clients + '/' + server.clients_max, 3.4)}} {{? !master.no_avgtop}} {{=constantWidth(Math.floor(server.pop_v) + '/' + server.clients_top, 3.4)}}{{?}} After: [...]

This isn't actually equivalent, it would become:

constantWidth(server.clients + "/" + server.clients_max, 3.4) +
!master.no_avgtop ? (constantWidth(Math.floor(server.pop_v) + '/' + server.clients_top, 3.4)) : "" +

Also here's a much more representative example, before:

{{? !master.no_version}}
<td class="version{{? server.mods && server.mods.length > 0}} mts_hover_list_text{{?}}">
    {{=escapeHTML(server.version)}}, {{=escapeHTML(server.gameid)}}
    {{? server.mapgen}}, {{=escapeHTML(server.mapgen)}}{{?}}
    {{=hoverList("Mods", server.mods)}}
</td>{{?}}

After:

if (!master.no_version) {
    var td = document.createElement("td");
    td.className = "version" + (server.mods && server.mods.length > 0 ? " mts_hover_list_text" : "");
    td.innerHTML =
        escapeHTML(server.version) + ", " +
        escapeHTML(server.gameid) + ", " +
        server.mapgen ? (", " + escapeHTML(server.mapgen)) : "" +
        hoverList("Mods", server.mods);
    tr.appendChild(td);
}

See how much simpler it is? I don't

Next, chrome has a built-in Javascript debugger (access through 'Inspect' entry of right click menu). [...]

I don't need an explanation what a debugger is, thanks.

The template on the other hand it either works or not - template is compiled to a blob.

A blob of JavaScript, you can debug it identically to a "pure" template. It's just less convenient.

And it requires knowing doT.

How many years did it take you?

Do we really need to pull node.js and other deps, then have a template pre-compilation step to deploy the server?

I hate installing node.js too, however I don't see it being worth the effort.

The master.no_ settings are meant to be set where list.js is included (ex index.html) and have not once been set or changed within the repository. Therefore the settings likely never had a real purpose.

You found where it's used yourself, yet you say it "has never had a real purpose". What?

The only other consumer (main is http://servers.minetest.net/) of server list functionality [...] is broken (Both Firefox and Chrome).

Thanks for noticing, I'll fix thatI've fixed that.

Further, bitrot / technical debt have acumulated and some parts of the master server, as seen above, do not even work. Instead of mechanically seeking parity with existing / previous code, maybe it is time for some rearchitecting. Is this functionality actually used or needed, because it does not seem so.

You can't just drop features because it's convenient.

What is the actual functionality that is missing?

Aside from the rest, here are two edge cases:

Since server list functionality is not touched, just the web table display, maybe it is worthwhile to take a risk, then go through a few design revisions if defects arise.

It's never worthwhile to push broken/incomplete code into production.