Best Practices

Follow these best practices when writing or reviewing userscript code.

DOM manipulation

Use addEventListener instead of “onevent”

onclick などの HTML 要素に “onevent” 値を設定することは避けてください。代わりにaddEventListener を使用してください。これにより、複数のアドオンが同じ要素に同じイベントを競合なく登録できます。 “onevent” を使用することは依然として有効ですが、イベントを登録している同じアドオンによって作成された要素に対してのみ有効です。。 すべての場合において、element.setAttribute("onclick", "don't do this") などの “onevent” HTML 属性の設定は避けてください。

// これをしないでください:
document.querySelector(".remix-button").onclick = () => {
  prompt("本当にリミックスしますか?");
};

// 代わりにこれを行ってください:
document.querySelector(".remix-button").addEventListener("click", () => {
  prompt("本当にリミックスしますか?");
});

Avoid using innerHTML

Avoid using innerHTML. Use innerText or textContent instead.
Other APIs that can potentially lead to XSS vulnerabilities should be avoided too, such as insertAdjacentHTML, outerHTML, document.write(), etc.

// Don't do this:
document.querySelector(".sa-remix-button").innerHTML = `<span>Remix ${projectTitle}</span>`;

// Do this instead:
const span = document.createElement("span");
span.textContent = `Remix ${projectTitle}`;
document.querySelector(".sa-remix-button").append(span);

Avoid using mousemove

頻繁にトリガーされる mousemove や類似の DOM イベントは、特に body で使用するとパフォーマンスが低下するため、使用を避けてください。可能なかぎり、子要素で代替イベントを使用してください。

// Don't do this:
body.addEventListener("mousemove", (event) => {
  // ...
});

Hide elements instead of removing them

Avoid calling .remove() on HTML elements, which in extreme cases, can cause the project page to crash.
Addons may only use it for elements they themselves created, in specific situations.

// Don't do this:
document.querySelector(".remix-button").remove();

// Do this instead:
document.querySelector(".remix-button").style.display = "none";

// Or do this, with help from a userstyle:
document.querySelector(".remix-button").classList.add("sa-remix-button-hidden");

Only use waitForElement when necessary

Avoid using the addon.tab.waitForElement API if the element is guaranteed to exist. It will still work, and performance will not be heavily impacted, but it might confuse other developers that are reading the code. The usage of waitForElement should usually mean that there is at least 1 scenario where the element doesn’t exist at that execution point.
For example, it’s not necessary to use waitForElement when searching for forum posts, unless the userscript was declared with "runAtComplete": false. In those cases, simply use document.querySelectorAll() normally.

Use element.closest() instead of abusing parentElement

Avoid overusing parentElement when traversing an element’s ancestors. Instead, use element.closest(), which works very similarly to element.querySelector().

// Don't do this:
reportButton.addEventListener("click", (event) => {
  const commentElement = event.target.parentElement.parentElement.parentElement.parentElement;
})

// Do this instead:
reportButton.addEventListener("click", (event) => {
  const commentElement = event.target.closest(".comment");
})

JavaScript best practices

Use modern JavaScript

• Prefer newer APIs, such as fetch() over XMLHttpRequest.
• Never use == for comparisons. Use === instead.
• When listening to keyboard events, accessing event.key is the preferred way to know which key was pressed. In general, you should avoid event.code and event.keyCode.
• Use optional chaining if an object can sometimes be null.
  For example, document.querySelector(".remix-button")?.textContent.
• Use for ... of loops or .forEach().
  Avoid C style loops like for (let i = 0; i < arr.length; i++).

Only use “let” over “const” if the variable may be reassigned

let actionCounter = 0;
actionCounter++;

const remixButton = document.querySelector(".remix-button");

const DEFAULT_ZOOM = 1.20;

People reading your code may assume that a variable that was declared through the “let” keyword might be reassigned at some other point of the script. If that’s not the case, use the “const” keyword instead.
Remember that in JavaScript, declaring an object or an array as a “const”, does not mean its values are frozen. Values in the object can still be changed, even if the variable itself cannot be reassigned.

Do not set global variables

Avoid setting properties on the global window object, unless you are polluting a global function such as fetch().
If multiple addons need to share information or functions between each other, create a JS module file and import it from both userscripts.

// Don't do this:
window.isDarkMode = true;

Do not declare functions outside of the default export

There’s no reason to declare functions outside the export default async function(){} function. JavaScript allows functions to be declared inside other functions.

You may move functions to separate JS module files (which aren’t declared as userscripts in the addon manifest) if appropriate, but keep in mind that those imported files won’t have access to the addon object, unless you expose a setup function that accepts it as an argument, and call the function in the userscript entry point.

Do not unpollute functions

Multiple addons might want to pollute the same function, such as Scratch VM methods, XMLHttpRequest, fetch() or FileReader().
In those cases, one of the userscripts will be polluting the real function, while the others will be polluting functions which were already polluted themselves. If, for example, the first userscript that polluted decides to unpollute (for example, by doing window.fetch = realFetch), then all other functions in the “pollution chain” are also lost, which is unexpected.
For this reason, functions should not be unpolluted. Instead, pass the arguments to the original function.

const oldDeleteSprite = vm.deleteSprite;
const newDeleteSprite = function (...args) {
  // ...
}

addon.self.addEventListener("disabled", () => {
  // Don't do this:
  vm.deleteSprite = oldDeleteSprite;
});

// Do this instead:
const oldDeleteSprite = vm.deleteSprite;
const newDeleteSprite = function (...args) {
  if (addon.self.disabled) return oldDeleteSprite.apply(this, args);
  // ...
};

Internationalization

Use addon.tab.scratchMessage()

If a string has already been translated by Scratch use addon.tab.scratchMessage instead of adding a new message.

// Don't do this:
doneButton.innerText = msg("done");

// Do this instead:
doneButton.innerText = addon.tab.scratchMessage("general.done");