Page MenuHomePhabricator

Remove ResourceLoader debug mode v1
Closed, ResolvedPublic

Description

Background

The original debug mode tries to load each file from its original place on disk. For example a request for load.php?module=foo, in debug moode, instead becomes three script tags like <script src=resources/foo/A.js> <script src=resources/foo/B.js> and <script src=resources/foo/C.js>. This was great because it means stack traces all reflect the original file paths, names, line numbers, etc, and of course naturally avoids minification.

There were two main drawbacks, which is why we developed debug mode v2 (T85805):

  • Behaviour difference. By loading files raw from disk, we skip all bundling transformations. This is desirable for transformations like minification (which you don't want when debugging). This is neutral for transformations like batching (meh). This is negative for logical transformations such as closure wrapping and package files, because it means local variables become global variables.
  • Speed. By loading each file raw, there is no closure wrapper. Without a closure wrapper, we cannot control when it executes. The script just executes as soon as the browser receives it. This means that we cannot load N files of the same module in parallel, and also means dependencies can't be loaded in parallel. The result is that each file is loaded serially with the browser only doing either downloading or parsing at any given time. There is a lot of idle time wasted there which means pages loaded slower in debug=1 mode. This was barely noticable in production for most workloads, but for certain exceptionally large extensions (e.g. VisualEditor) this would add several whole seconds of unavoidable delay.

Since then we have developed debug mode v2 (T85805), which is essentially unmodified production mode with only batching and minification turned off. This means code is easily identifitiable, is not combined with unrelated modules, and maintains concurrency for speed.

Further more, we have shipped support for Source Maps (T47514: ResourceLoader: Implement support for Source Maps) which means that even in production mode, you basically get the "perfect" debug mode without needing to even turn on debug mode.

Related:

Problem

  1. The experience we serve when turning on debug=true is not the recommended debugging experience. This increases the learning curve and means people run into issues we already solved.

When asked directly, we generally recommend developers to open the devtools directly in production mode, because all modern browsers will automatically apply ResourceLoader's source maps. No need to enable debug mode. Or if you want to enable other debug features (e.g. verbose logging, and disabling localStorage), I generally recommend debug=2 not debug=true.

  1. The existence of debug mode v1 justifies a fear of leaking global variables. The result is that people (understandably) defensively wrap each JS file in a closure just in case, and may toggle ESLint rules such as no-implicit-globals.

This is something that was meant to be in the past after T50886: ResourceLoader: Remove obsolete closures from files, but as long as our ESLint present does not reflect this, people will likely continue to think that these workarounds are still needed!

Proposal

  • Promote debug=2 as the mapping for debug=true. Today it maps to debug=1.
  • After a few weeks, remove debug=1.
  • Update eslint-config-wikimedia to default to commonjs mode since both package modules and classic modules are always wrapped in commonjs closures by ResourceLoader, there is no risk of leaking variables. The only reason we adopted this convention back in 2010 is because in debug mode, this closure would be omitted. This will not be the case anymore if we remove debug=1.

In communication, it'll be important to emphasize that source maps transparently tell the browser the original location of your code, without needing to actually re-download it from that URL. This means all production behaviours and transformations reliably apply, creating consistency between production mode, debug mode, and debugging with source maps.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Update eslint-config-wikimedia to default to commonjs mode

...within the /mediawiki preset. Our core rules are stylistic and environment-agnostic.

Krinkle added a subscriber: Hokwelum.

Might get to this next quarter with @Hokwelum as part of focussing on MW tech debt and onboarding with RL, along with T343492: Phase out SqlModuleDependencyStore.

This week someone experienced another old bug with debug v1, which wasted time in debugging and even led to a bug fix to improve debug v1:

[mediawiki/core] ResourceLoader: Treat styles in RTL mode as "generated| to bypass raw v1 debug.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1062043

It looks like, maybe, if debug v1 is removed, we can also look at removing the internal hasGeneratedStyles and getStyleURLsForDebug concepts.

Change #1115825 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] Debug=true now maps to v2 debug mode

https://gerrit.wikimedia.org/r/1115825

Change #1115825 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Debug mode now defaults to debug mode v2

https://gerrit.wikimedia.org/r/1115825

Hokwelum changed the task status from Open to In Progress.Feb 10 2025, 3:11 PM

Change #1119617 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] ResourceLoader: Remove Debug mode v1

https://gerrit.wikimedia.org/r/1119617

Change #1119617 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Remove Debug mode v1

https://gerrit.wikimedia.org/r/1119617

Change #1120251 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] resourceloader: Remove getScriptURLsForDebug()

https://gerrit.wikimedia.org/r/1120251

Change #1120251 merged by jenkins-bot:

[mediawiki/core@master] resourceloader: Remove getScriptURLsForDebug()

https://gerrit.wikimedia.org/r/1120251

Re-opening for the ESLint TODO.

The PR has been merged, and is now released by James as part of eslint-config-wikimedia 0.29.

https://github.com/wikimedia/eslint-config-wikimedia/blob/v0.29.0/CHANGELOG.md

Change #1126566 had a related patch set uploaded (by Krinkle; author: Máté Szabó):

[mediawiki/core@master] ResourceLoader: Remove unused ResourceLoader::expandUrl() method

https://gerrit.wikimedia.org/r/1126566

Change #1126566 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Remove unused ResourceLoader::expandUrl() method

https://gerrit.wikimedia.org/r/1126566

I would have thought that "remove" means that debug=1 behaves the same as debug=true, but actually, it doesn't enable debug mode at all (e.g. mw.log() doesn't work). That feels pretty unintuitive.

@Tgr The idiom has generally been debug=true, which continues to work and uses v2 nowadays.

The debug=1 parameter was a temporary stopgap for activating v1, after v2 was introduced. It seemed confusing for that to activate v2, so I removed it. Was the transition long enough, and the need to use v1 common enough, to enter muscle memory? Or is debug=1 hardcoded somewhere that it shouldn't be?

Change #1192175 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/vagrant@master] buggy: Use debug=true instead of debug=1 in "Debug mode" links

https://gerrit.wikimedia.org/r/1192175

Change #1192176 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/MobileFrontend@master] tests: Remove unrelated debug=1 from example URL

https://gerrit.wikimedia.org/r/1192176

Change #1192177 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] linker: Update outdated LinkRenderer $query example

https://gerrit.wikimedia.org/r/1192177

Change #1192175 merged by Gergő Tisza:

[mediawiki/vagrant@master] buggy: Use debug=true instead of debug=1 in "Debug mode" links

https://gerrit.wikimedia.org/r/1192175

Change #1192177 merged by jenkins-bot:

[mediawiki/core@master] linker,tests: Update outdated debug=1 in examples

https://gerrit.wikimedia.org/r/1192177

Change #1192176 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] tests: Remove unrelated debug=1 from example URL

https://gerrit.wikimedia.org/r/1192176

I think I have always used debug=1; it's just the more natural way of enabling a flag (especially if coming from the PHP world). Maybe worth detecting it and adding a warning to the console?

I see your point. The debug parameter is primarily a boolean param. People should only need to know its name and expect it to work like any other boolean param in MediaWiki. That is why debug=true has always been the canonical way we describe it in docs, and the version numbers merely a transition mechanism for those in the know wanting to opt-in or out.

When removing v1, it seemed important to not create an near-miss for the subgroup that got used to opting-out of v2 by setting debug=1 (before it was ready for their use case) and nudge them back to debug=true with fresh expectations.

But, other boolean params like safemode, printable, and others using WebRequest::getFuzzyBool in MediaWiki all support both true/false and 0/1, so if you got used to safemode=1, you reasonably expect debug=1 to work. If one day we introduce a debug=3 and drop debug=2, I might argue that removing debug=2 is right for that. But I think debug=1 makes sense as alias for true indeed (not as alias for 2).

Change #1215694 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] ResourceLoader: Restore debug=1 as alias for debug=true

https://gerrit.wikimedia.org/r/1215694

Change #1215694 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Restore debug=1 as alias for debug=true

https://gerrit.wikimedia.org/r/1215694