Refactor getQueryParameters() to use URL/URLSearchParams

[shaedrich]

Would it make sense to refactor getQueryParameters() to use URL/URLSearchParams, so we don't have to maintain custom code over native one or do you prefer not to fix something that ain't broken?

pony-playground/static/web.js

Lines 295 to 305 in 35f1d18

	function getQueryParameters() {
	var a = window.location.search.substr(1).split('&');
	if (a === "") return {};
	var b = {};
	for (var i = 0; i < a.length; i++) {
	var p = a[i].split('=');
	if (p.length != 2) continue;
	b[p[0]] = decodeURIComponent(p[1].replace(/\+/g, " "));
	}
	return b;
	}

By the way, this

pony-playground/static/web.js

Lines 7 to 12 in 35f1d18

	function optionalLocalStorageGetItem(key) {
	try {
	return localStorage.getItem(key);
	} catch (e) {
	return null;
	}

could also be rewritten as

function optionalLocalStorageGetItem(key) {
    return localStorage.getItem(key) ?? null;
}

and so can

pony-playground/static/web.js

Lines 496 to 501 in 35f1d18

	theme = optionalLocalStorageGetItem("theme");
	if (theme === null) {
	set_theme(editor, themelist, "GitHub");
	} else {
	set_theme(editor, themelist, theme);
	}

as

theme = optionalLocalStorageGetItem("theme");
set_theme(editor, themelist, theme ?? "GitHub");

as well as

pony-playground/static/web.js

Lines 25 to 33 in 35f1d18

	var themes = themelist.themes;
	themes.sort(function (a, b) {
	if (a.caption < b.caption) {
	return -1;
	} else if (a.caption > b.caption) {
	return 1;
	}
	return 0;
	});

which can be shortened to

var themes = themelist.themes;
themes.sort((a, b) => a.caption.localeCompare(b.caption));

Then, we have

pony-playground/static/web.js

Lines 116 to 121 in 35f1d18

	var classes = mappings[arguments[1]];
	if (classes) {
	return '<span class="' + classes + '">' + arguments[2] + '</span>';
	} else {
	return arguments[2];
	}

which doesn't need the else, since it has a return:

var classes = mappings[arguments[1]];
if (classes) {
    return '<span class="' + classes + '">' + arguments[2] + '</span>';
}
return arguments[2];

The following is an interesting case:

pony-playground/static/web.js

Lines 251 to 258 in 35f1d18

	if (on_success) {
	on_success(req.responseText);
	}
	} else {
	if (on_fail) {
	on_fail(req.status, req.responseText);
	}
	}

it could either be rewritten as

if (req.status == expect) {
    if (on_success) {
        on_success(req.responseText);
    }
} else if (on_fail) {
    on_fail(req.status, req.responseText);
}

or

if (req.status == expect && on_success) {
    on_success(req.responseText);
}
if (req.status != expect && on_fail) {
    on_fail(req.status, req.responseText);
}

Next, we have a bunch of these:

pony-playground/static/web.js

Lines 617 to 627 in 35f1d18

	asmButton.onclick = function () {
	compile("asm", result, session.getValue(), asmButton);
	};
	irButton.onclick = function () {
	compile("llvm-ir", result, session.getValue(), irButton);
	};
	gistButton.onclick = function () {
	shareGist(result, session.getValue(), gistButton);
	};

when they only contain a single function call, instead of creating a bunch of overhead with new anonymous wrapper functions, you could use bind:

asmButton.onclick = compile.bind(window, "asm", result, session.getValue(), asmButton);

irButton.onclick = compile.bind(window, "llvm-ir", result, session.getValue(), irButton);

gistButton.onclick = shareGist.bind(window, result, session.getValue(), gistButton);

clearResultButton.onclick = clear_result.bind(window, result);

themes.onkeyup = themes.onchange = set_theme.bind(window, editor, themelist, themes.options[themes.selectedIndex].text);

request.ontimeout = set_result.bind(window, result, "<p class=error>Connection timed out" +
                "<p class=error-explanation>Are you connected to the Internet?");

One could possibly also refactor editGo(), editShowRegion(), editShowLine(), and editShowPoint(), but that'd be slightly more than a simplification.

Miscellaneous

• className vs classList
• fetch vs XMLHttpRequest
• setAttribute('data-*') vs dataset
• onclick vs addEventListener()

[jemc]

@mfelsche - are we still pulling in updated code from the Rust playground that we lifted from (in which case it may not make sense to make a bunch of maintainability-focused changes to the javascript code) or are we pretty decoupled from that upstream repo now (in which case it may indeed make sense to make iterative small improvements)?

[mfelsche]

@jemc we didnt update it from the original codebase for ages. I didnt do any work on the frontend bits, but would be happy to get those on stable feet. Happy for every improvement here.

[shaedrich]

Am I missing something or is samples obsolete and can be removed?

pony-playground/static/web.js

Line 5 in 14abb9e

var samples = 2;

[mfelsche]

It doesn't seem to be referenced. Your js language server of choice might know more through some static analysis.

[shaedrich]

Would you like to have JSDoc comments above your functions?

[mfelsche]

Those aren't my functions. They belong to everyone. Treat them like they are yours. Whatever improves code quality, Iwe are happy to accept. I can write Rust and Pony, but I don't really dare to touch javascript.
So I hereby hand ownership of the JS code in here to you. It is old and hungry and copied and only slightly adapted from the rust playground it was forked from. Feel free to improve in any way you deem helpful (mostly to you doing further improvements).

[shaedrich]

Those aren't my functions. They belong to everyone.

Yeah, sorry, bad wording on my part 😅 I didn't mean singular "you". More like that the repository owners might have a little more say in this

Treat them like they are yours. Whatever improves code quality, Iwe are happy to accept. […] So I hereby hand ownership of the JS code in here to you. […] Feel free to improve in any way you deem helpful (mostly to you doing further improvements).

Perfect 👍🏻 Will do.