Modernize bind example #4
Conversation
There was a problem hiding this comment.
Thanks for this PR but it's unlikely to be considered for merge even with fixes based on my feedback.
- I suggest logic (JS) stays separate from presentation (HTML) and not inline the scripts within attributes of HTML elements.
I suggest not inlining styles either but instead put them in proper(Oops, duplicated point.)styleelements.- With that said, adding styles isn't necessary for this example.
querySelectorAll(andquerySelector) is more modern thangetElementByIdbut with only one ID to look up, I recognizegetElementByIdis probably more efficient.- Inconsistent usage of semicolon to end statements.
- Why was
incrementrenamed toIncrement?window.Increment()isn't consistent with JS code style. - I recognize
async/awaitis modern, and I recognize that a newasyncfunction is required because you can't useawaitin the global scope unless thescripttag is marked withtype="module", but the naming is confusing (increment()andIncrement()both existing onwindow). - Why was handling of
DOMContentLoadedremoved? I recognize that in the existing example, thescriptelement would be withinbodyat the end and thereforeDOMContentLoadedwould be unnecessary. It would have been necessary had thescriptelement been withinhead. - I assume you didn't mean to change indentations from spaces to tabs.
If this suggests not using <button id="increment">Tap me</button>
<div>You tapped <span id="count">0</span> time(s).</div>
<script>
const [incrementElement, countElement] =
document.querySelectorAll("#increment, #count");
incrementElement.addEventListener("click", () => {
window.increment().then(result => {
countElement.textContent = result.count;
});
});
</script>over <button onclick="increment();">Tap me</button>
<div>You tapped <span id="count">0</span> time(s).</div>
<script>
const counter = document.getElementById("count")
async function increment() {
const result = await window.Increment();
counter.textContent = result.count;
}
</script>Then we have to agree that we disagree.
Not inlining style is definitely a best practice that should be followed. Since this is already a partial example, without "proper elements" it was kept short and a
Both are okay I think. The change was not about using a more modern function.
Good catch. A mistake I made that should be corrected.
It was renamed to not interfere with the
Yes, that's the reason. Also if this would be an extended example the best practice would be to keep this script at the end of the body.
An unintended change. But none to regret I think. Tabs offer increased accessibility and adapt to a users indentation width. Spaces for indentation can fit in code presentations or in stdouts, where thinks should be forced to look a certain way. In real code they are bugs, as they don't adapt to developer preferences and making codebases for people with some impairments inaccessible. It could fit into a go example as go code uses tabs. |
|
Thanks for taking the time to answer all of my points! This is a bit long but I would like to give a proper response.
Yes. Something like this could also be considered, and has the same line count as your example: <button id="increment">Tap me</button>
<div>You tapped <span id="count">0</span> time(s).</div>
<script>
const [incrementElement, countElement] =
document.querySelectorAll("#increment, #count");
incrementElement.addEventListener("click", async () => {
countElement.textContent = (await window.increment()).count;
});
</script>It's generally a good practice to avoid polluting the global scope, and the solution to this is to either add module scope or function scope. When adding the scope then code using <button onclick="onIncrementClick()">Tap me</button>
<div>You tapped <span id="count">0</span> time(s).</div>
<script type="module">
const counter = document.getElementById("count")
async function onIncrementClick() {
const result = await window.increment();
counter.textContent = result.count;
}
</script><button onclick="onIncrementClick()">Tap me</button>
<div>You tapped <span id="count">0</span> time(s).</div>
<script>
(() => {
const counter = document.getElementById("count")
async function onIncrementClick() {
const result = await window.increment();
counter.textContent = result.count;
}
});
</script>Neither of these examples will work because We can of course work around this but if it still pollutes the global scope then it also allows other scripts to invoke There are definitely things that could be improved including scoping as shown above, strict mode and HTML correctness. We should also eventually update the example to demonstrate long-running tasks. See bind.cc and webview/webview#932.
I suggest not adding styles here unless we go all the way. We can save it for a more complete example with proper HTML, styles, scripts, maybe loading of embedded resources, frameworks, etc. All that has to be done in the core library's repository as well and show the features of the core library.
I normally prefer With that said, I did a benchmark with WebKit2GTK 2.40.5 on Ubuntu 22.04.3 because I wanted to know for sure (see result below). Negliglble differences in the real world, but might as well use
I agree that this should be removed.
We should keep web code and behavior consistent with the examples in the core library's repository—well, the previous example that didn't demonstrate long-running tasks. Benchmark
Result: Code: <!DOCTYPE html>
<html lang="en">
<head>
<title>Benchmark</title>
</head>
<body>
<div id="a"></div>
<div id="b"></div>
<div id="c"></div>
<div id="d"></div>
<script type="module">
function benchmark(name, iterations, work) {
const start = window.performance.now();
for (let i = 0; i < iterations; ++i) {
work();
}
const end = window.performance.now();
const duration = end - start;
console.log(`${name}:
total = ${duration} ms
per iteration = ${(duration / iterations) * Math.pow(10, 6)} ns`);
}
const iterations = 100_000_000;
benchmark("getElementById", iterations, () => {
document.getElementById("a");
document.getElementById("b");
document.getElementById("c");
document.getElementById("d");
});
benchmark("querySelector", iterations, () => {
document.querySelector("#a");
document.querySelector("#b");
document.querySelector("#c");
document.querySelector("#d");
});
benchmark("querySelectorAll", iterations, () => document.querySelectorAll("#a, #b, #c, #d"));
</script>
</body>
</html> |
|
Expanded the "unless it's in a frozen object" part to consider |
|
Thanks for the effort @SteffenL. Seeing the benches, In terms of the example. It's comes down to preferences, I think. Then nuances and differentiation's of what is appropriate for an example and has good readability are mixed in. I.e, I think using globally scoped functions, with the goal of trying to keep things as clean and readable as possible while focusing on highlighting the interoperability with the library is just fine in a small example like this, and makes it even simpler to follow. But what is clean and readable comes also down to preference. All the arguments are of course correct. So I can't tell that there is a universally better version of the example and don't want to interfere too much here. The decision is down to the creator / core maintainer of a project. I'm just as fine with the PR being closed or updated, I am not attached to the suggestion in the PR. No worries 👍 . |
ttytm
force-pushed
the
examples/modernize-bind
branch
2 times, most recently
from
d65675b to
ba0307e
Compare
ttytm
force-pushed
the
examples/modernize-bind
branch
from
ba0307e to
f9596df
Compare
No description provided.