fix(perf): use classList.toggle instead of add/remove

[akx]

BREAKING CHANGE: classList.toggle is used now

This is a breaking change for everyone who needs to support very old browsers which don't support the second argument of classList.toggle, like IE. If you need to support those browsers you need to add a polyfill like https://github.com/eligrey/classList.js

PR description

• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• Run the tests with npm test and lint the project with npm run lint

---

I noticed an app (stable-diffusion-webui, built with gradio which is built with svelte) was spending 6.3% of its time in toggle_class, and figured it'd likely perform better without a dynamic property access.

classList.toggle(..., flag) has been a part of the DOM standard forever, so better use it instead of possibly causing browser deopts by using dynamic attribute access. The !! is required because an undefined flag means flipping the current state.

This could of course be ignored if some of the browsers that don't support the force flag are still targeted by Svelte.

[vercel]

@akx is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[Conduitry]

Svelte 3 does officially still support IE11, with certain transpilation and polyfills (for Map, Set, etc.). We want to avoid using features of APIs where the developer would have to detect whether they supported a particular feature, instead of just being able to polyfill them if they are absent.

The supported list of browsers might be changing in Svelte 4. I'm not sure what the plans are there, actually.

[Conduitry]

This has also come up before in #3189, I just noticed. We still don't want to make this change in Svelte 3. We probably do want to make it in Svelte 4.

[benmccann]

It'd be good to get a benchmark to show that this change is worth it if we do merge it

We'd need to note in the changelog that this would need to be polyfilled for legacy browsers (e.g. using https://github.com/lukeed/classico)

[akx]

It'd be good to get a benchmark to show that this change is worth it if we do merge it

Sure. With https://gist.github.com/akx/97b237ae55589e7a166ab88c913b418e I get

chrome 113

(20% speedup)

firefox 113

(not much of a difference)

safari 16.4

(pretty shocking difference here tbh)

[dummdidumm]

Out of curiosity, does writing add ? element.classList.add(name) : element.classList.remove(name); instead of that string access make any difference in the perf tests?

[akx]

@dummdidumm Updated gist: https://gist.github.com/akx/76818fb782a232b5e08eae785cb3ee97

Using toggle is still fastest, but using if (or a ternary) is a bit faster than the original code.

On that note, @Conduitry & @benmccann: I can rework this PR to just do

    function toggle_class(element, name, toggle) {
        if (toggle) {
            element.classList.add(name);
        } else {
            element.classList.remove(name);
        }
    }

so remaining compatible with all browsers, and we'd still get a perf increase of ~10% (as opposed to ~15% with using toggle).

WDYT? See #8631 for that take.

[Rich-Harris]

Thanks! I personally lean towards this over #8631 — shaving bytes off feels more important than retaining compability with browsers where other stuff is almost guaranteed to be broken anyway