Modernize bind example

[ttytm]

• I suggest logic (JS) stays separate from presentation (HTML) and not inline the scripts within attributes of HTML elements.

If this suggests not using onclick and use an event listener like

<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.

• I suggest not inlining styles either but instead put them in proper style elements.
• With that said, adding styles isn't necessary for this example.

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 head and style element were not added. I also agree that styles ar not necessary, it was only added to make things a little easier on the eyes. Assuming a lot of developer appreciate not being flashed by white windows.

• querySelectorAll (and querySelector) is more modern than getElementById but with only one ID to look up, I recognize getElementById is probably more efficient.

Both are okay I think. The change was not about using a more modern function. getElementById() remained a personal preference to use when querying elements by id as it has more than double the performance. This does not mean querySelector is not performant. The performance difference for querying elements is probably insignificant for most application. Some even recommend using querySelector for the single id query operation as it is a uniform way to query elements.

• Inconsistent usage of semicolon to end statements.

Good catch. A mistake I made that should be corrected.

• Why was increment renamed to Increment? window.Increment() isn't consistent with JS code style.
• I recognize async/await is modern, and I recognize that a new async function is required because you can't use await in the global scope unless the script tag is marked with type="module", but the naming is confusing (increment() and Increment() both existing on window).

It was renamed to not interfere with the async function increment. It's true that it breaks with the JS camelCalse convention. Since this is a go example I classified using Gos UpperCamelCase for an exported "API" function as okay. If that's not preferred the added JS function can be named something like handleIncrement.

• Why was handling of DOMContentLoaded removed? I recognize that in the existing example, the script element would be within body at the end and therefore DOMContentLoaded would be unnecessary. It would have been necessary had the script element been within head.

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.

• I assume you didn't mean to change indentations from spaces to tabs.

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.

[SteffenL]

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.

• I suggest logic (JS) stays separate from presentation (HTML) and not inline the scripts within attributes of HTML elements.

If this suggests not using onclick and use an event listener like [...]

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 addEventListener will continue to work but code with onclick will not:

<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 onIncrementClick is inaccessible.

We can of course work around this but if it still pollutes the global scope then it also allows other scripts to invoke onIncrementClick, and even replace it unless it's in a frozen object assigned to a const variable, or a function assigned to a const variable.

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 https://github.com/webview/webview/pull/932.

Not inlining style is definitely a best practice that should be followed

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.

[...] getElementById() remained a personal preference to use when querying elements by id as it has more than double the performance. [...]

I normally prefer getElementById for the same reason although we probably won't see any difference outside of benchmarks.

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 getElementById over querySelector if used for the same job, but I would say that querySelectorAll has its place even for the same job (potentially less duplicate code).

• Why was handling of DOMContentLoaded removed? I recognize that in the existing example, the script element would be within body at the end and therefore DOMContentLoaded would be unnecessary. [...]

Yes, that's the reason. [...]

I agree that this should be removed.

• I assume you didn't mean to change indentations from spaces to tabs.

An unintended change. [...] It could fit into a go example as go code uses tabs.

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

• 100_000_000 iterations.
• 4 different elements to look up by ID.
• getElementById and querySelector invoked for each element per iteration (individual statements/not looped).
• querySelectorAll invoked once per iteration.

Result:

getElementById:
  total         = 8502 ms
  per iteration = 85.02 ns
querySelector:
  total         = 30512 ms
  per iteration = 305.12 ns
querySelectorAll:
  total         = 33162 ms
  per iteration = 331.62 ns

Number of iterations per 1 ms:
  getElementById  : 11_761
  querySelector   : 3_277
  querySelectorAll: 3_015

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>

[SteffenL]

Expanded the "unless it's in a frozen object" part to consider const variables.

[ttytm]

Thanks for the effort @SteffenL.

Seeing the benches, getElementById performs better than I remember.

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 :+1: .