Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

ResourceLoader: Default File modules to mobile and desktop targets

Browse source 
If not specified, going forward all modules which do not define
targets should be assumed to be mobile AND desktop. I think this is
a fair expectation given that most new features should be built with
mobile in mind. This also avoids the issue where features are
unexpectedly not shipped to mobile.

The work in T324723 has ensured that all modules that need to remain
desktop only have been marked as such. After the audit, we can be
reasonably confident that enabling the remaining modules is okay,
given these modules run in isolated parts of the UI (not on every page
view).

I'm assuming CI is catching all offenders here. If not, any impacted
modules would require a trivial update to explicitly define modules
as mobile only. This would allow us to see clearly which modules
are depending on the target system and strategize around updating
them.

Once this patch is merged we can focus on fixing the modules we've
identified as mobile or desktop targeted and finally dismantling the
targets system altogether.

Bug: T127268
Change-Id: Ia062ff2d8b8732b0d3498c1a614bbf6a3e3a7ddb
This commit is contained in:
Jon Robson 2022-12-07 17:11:34 -08:00 committed by Jdlrobson
parent 6ebdb3d20a
commit 01aec63c21
4 changed files with 17 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/ResourceLoader/FileModule.php
View file

@ -133,7 +133,7 @@ class FileModule extends Module {
protected $debugRaw = true;
/** @var string[] */
protected $targets = [ 'desktop' ];
protected $targets = [ 'desktop', 'mobile' ];
/** @var bool Whether CSSJanus flipping should be skipped for this module */
protected $noflip = false;

2
includes/registration/ExtensionProcessor.php
View file

@ -647,6 +647,8 @@ class ExtensionProcessor implements Processor {
$data['localBasePath'] = "$dir/{$data['localBasePath']}";
}
}
// Satisfy PHPUnit ResourcesTest::testUnsatisfiableDependencies
$data['targets'] = [ 'test' ];
$this->attributes['QUnitTestModules']["test.{$info['name']}"] = $data;
}

19
tests/phpunit/structure/ResourcesTest.php
View file

@ -110,6 +110,8 @@ class ResourcesTest extends MediaWikiIntegrationTestCase {
$data = self::getAllModules();
/** @var RL\Module $module */
$incompatibleTargetNames = [];
$targetsErrMsg = '';
foreach ( $data['modules'] as $moduleName => $module ) {
$depNames = $module->getDependencies( $data['context'] );
$moduleTargets = $module->getTargets();
@ -124,14 +126,18 @@ class ResourcesTest extends MediaWikiIntegrationTestCase {
// Missing dependencies reported by testMissingDependencies
continue;
}
if ( $moduleTargets === [ 'test' ] ) {
// Target filter does not apply under tests, which may include
// both module-only and desktop-only dependencies.
continue;
}
$targets = $dep->getTargets();
foreach ( $moduleTargets as $moduleTarget ) {
$this->assertContains(
$moduleTarget,
$targets,
"The module '$moduleName' must not have target '$moduleTarget' "
. "because its dependency '$depName' does not have it"
);
if ( !in_array( $moduleTarget, $targets ) ) {
$incompatibleTargetNames[] = $moduleName;
$targetsErrMsg .= "* The module '$moduleName' must not have target '$moduleTarget' "
. "because its dependency '$depName' does not have it\n";
}
}
if ( !$requiresES6 && $dep->requiresES6() ) {
$incompatibleDepNames[] = $depName;
@ -141,6 +147,7 @@ class ResourcesTest extends MediaWikiIntegrationTestCase {
"The module '$moduleName' must not depend on modules with requiresES6=true"
);
}
$this->assertEquals( [], $incompatibleTargetNames, $targetsErrMsg );
}
/**

1
tests/phpunit/unit/includes/registration/ExtensionProcessorTest.php
View file

@ -901,6 +901,7 @@ class ExtensionProcessorTest extends MediaWikiUnitTestCase {
'localBasePath' => $dir,
'remoteExtPath' => 'Foo',
'scripts' => 'bar.js',
'targets' => [ 'test' ],
],
],
],