Skip to content

Allow apps to include module js / ES6 scripts #36057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
juliusknorr merged 5 commits into from
Mar 22, 2023
Merged

Allow apps to include module js / ES6 scripts #36057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dbb1fa1
fix(OC/Template): Remove usage of deprecated functions in `JSResource…
susnux Jan 10, 2023
00e041b
feat(template): Allow `JSResourceLocator` to find ES6 scripts
susnux Jan 10, 2023
b642137
feat(OC/Template): Add `type="module"` for ES6 scripts
susnux Jan 10, 2023
a3595f7
fix(OC/Template): Allow `.mjs` files within custom app paths
susnux Jan 10, 2023
d461da3
Adjust `.htaccess` to serve `.mjs` files with javascript mime type
susnux Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .htaccess
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@
AddType image/svg+xml svg svgz
AddType application/wasm wasm
AddEncoding gzip svgz
# Serve ESM javascript files (.mjs) with correct mime type
AddType text/javascript js mjs
</IfModule>

<IfModule mod_dir.c>
Expand Down
93 changes: 58 additions & 35 deletions lib/private/Template/JSResourceLocator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,19 @@
*/
namespace OC\Template;

use OCP\App\AppPathNotFoundException;
use OCP\App\IAppManager;
use Psr\Log\LoggerInterface;

class JSResourceLocator extends ResourceLocator {
/** @var JSCombiner */
protected $jsCombiner;
protected JSCombiner $jsCombiner;
protected IAppManager $appManager;

public function __construct(LoggerInterface $logger, JSCombiner $JSCombiner) {
public function __construct(LoggerInterface $logger, JSCombiner $JSCombiner, IAppManager $appManager) {
parent::__construct($logger);

$this->jsCombiner = $JSCombiner;
$this->appManager = $appManager;
}

/**
Expand All @@ -53,59 +56,63 @@ public function doFind($script) {
// For language files we try to load them all, so themes can overwrite
// single l10n strings without having to translate all of them.
$found = 0;
$found += $this->appendIfExist($this->serverroot, 'core/'.$script.'.js');
$found += $this->appendIfExist($this->serverroot, $theme_dir.'core/'.$script.'.js');
$found += $this->appendIfExist($this->serverroot, $script.'.js');
$found += $this->appendIfExist($this->serverroot, $theme_dir.$script.'.js');
$found += $this->appendIfExist($this->serverroot, 'apps/'.$script.'.js');
$found += $this->appendIfExist($this->serverroot, $theme_dir.'apps/'.$script.'.js');
$found += $this->appendScriptIfExist($this->serverroot, 'core/'.$script);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'core/'.$script);
$found += $this->appendScriptIfExist($this->serverroot, $script);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.$script);
$found += $this->appendScriptIfExist($this->serverroot, 'apps/'.$script);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'apps/'.$script);

if ($found) {
return;
}
} elseif ($this->appendIfExist($this->serverroot, $theme_dir.'apps/'.$script.'.js')
|| $this->appendIfExist($this->serverroot, $theme_dir.$script.'.js')
|| $this->appendIfExist($this->serverroot, $script.'.js')
|| $this->appendIfExist($this->serverroot, $theme_dir . "dist/$app-$scriptName.js")
|| $this->appendIfExist($this->serverroot, "dist/$app-$scriptName.js")
|| $this->appendIfExist($this->serverroot, 'apps/'.$script.'.js')
} elseif ($this->appendScriptIfExist($this->serverroot, $theme_dir.'apps/'.$script)
|| $this->appendScriptIfExist($this->serverroot, $theme_dir.$script)
|| $this->appendScriptIfExist($this->serverroot, $script)
|| $this->appendScriptIfExist($this->serverroot, $theme_dir."dist/$app-$scriptName")
|| $this->appendScriptIfExist($this->serverroot, "dist/$app-$scriptName")
|| $this->appendScriptIfExist($this->serverroot, 'apps/'.$script)
|| $this->cacheAndAppendCombineJsonIfExist($this->serverroot, $script.'.json')
|| $this->appendIfExist($this->serverroot, $theme_dir.'core/'.$script.'.js')
|| $this->appendIfExist($this->serverroot, 'core/'.$script.'.js')
|| (strpos($scriptName, '/') === -1 && ($this->appendIfExist($this->serverroot, $theme_dir . "dist/core-$scriptName.js")
|| $this->appendIfExist($this->serverroot, "dist/core-$scriptName.js")))
|| $this->appendScriptIfExist($this->serverroot, $theme_dir.'core/'.$script)
|| $this->appendScriptIfExist($this->serverroot, 'core/'.$script)
|| (strpos($scriptName, '/') === -1 && ($this->appendScriptIfExist($this->serverroot, $theme_dir."dist/core-$scriptName")
|| $this->appendScriptIfExist($this->serverroot, "dist/core-$scriptName")))
|| $this->cacheAndAppendCombineJsonIfExist($this->serverroot, 'core/'.$script.'.json')
) {
return;
}

$script = substr($script, strpos($script, '/') + 1);
$app_path = \OC_App::getAppPath($app);
$app_url = \OC_App::getAppWebPath($app);
$app_url = null;

try {
$app_url = $this->appManager->getAppWebPath($app);
} catch (AppPathNotFoundException) {
// pass
}

try {
$app_path = $this->appManager->getAppPath($app);

if ($app_path !== false) {
// Account for the possibility of having symlinks in app path. Only
// do this if $app_path is set, because an empty argument to realpath
// gets turned into cwd.
$app_path = realpath($app_path);
}

// missing translations files fill be ignored
if (strpos($script, 'l10n/') === 0) {
$this->appendIfExist($app_path, $script . '.js', $app_url);
return;
}
// missing translations files will be ignored
if (strpos($script, 'l10n/') === 0) {
$this->appendScriptIfExist($app_path, $script, $app_url);
return;
}

if ($app_path === false && $app_url === false) {
if (!$this->cacheAndAppendCombineJsonIfExist($app_path, $script.'.json', $app)) {
$this->appendScriptIfExist($app_path, $script, $app_url);
}
} catch (AppPathNotFoundException) {
$this->logger->error('Could not find resource {resource} to load', [
'resource' => $app . '/' . $script . '.js',
'app' => 'jsresourceloader',
]);
return;
}

if (!$this->cacheAndAppendCombineJsonIfExist($app_path, $script.'.json', $app)) {
$this->append($app_path, $script . '.js', $app_url);
}
}

Expand All @@ -115,14 +122,30 @@ public function doFind($script) {
public function doFindTheme($script) {
}

/**
* Try to find ES6 script file (`.mjs`) with fallback to plain javascript (`.js`)
* @see appendIfExist()
*/
protected function appendScriptIfExist(string $root, string $file, string $webRoot = null) {
Copy link
Contributor

@come-nc come-nc Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have performance implications as it will always search first for a .mjs before searching for the .js? Is the result cached?

Copy link
Contributor Author

@susnux susnux Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will take twice the time, but I do not know if checking if a file exists is really that expensive.
Checking mjs first is required for backwards compatibility. This way you can provide .js files for older Nextcloud releases and .mjs files for NC27. So your app can support multiple versions.

Of cause one could add a locator cache, but I am not sure how to invalidate the cache.

if (!$this->appendIfExist($root, $file . '.mjs', $webRoot)) {
Copy link
Member

@PVince81 PVince81 Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of the changes above that prevent throwing AppPathNotFoundException it could happen that $root and $webRoot are null

to preserve the original behavior you'd likely need to throw that exception here after checking

Copy link
Contributor Author

@susnux susnux Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original behavior, as previously \OC_App::getAppPath was used which did not trow but return false if not found.
So this behavior is kept by assigning false to $app_path and $app_url and just ignore AppPathNotFoundException above (if it is raised, the value would stay false).

If $app_path is false then appendIfExist does skip it and if both are false, then this is handled by the line 115 (next code below the if(strpos` block)

if ($app_path === false && $app_url === false) {
// ...

But I agree that this is not that obvious from the code without digging into it. I tried to make this more clear and self explaining (see latest commit).

return $this->appendIfExist($root, $file . '.js', $webRoot);
}
return true;
}

protected function cacheAndAppendCombineJsonIfExist($root, $file, $app = 'core') {
if (is_file($root.'/'.$file)) {
if ($this->jsCombiner->process($root, $file, $app)) {
$this->append($this->serverroot, $this->jsCombiner->getCachedJS($app, $file), false, false);
} else {
// Add all the files from the json
$files = $this->jsCombiner->getContent($root, $file);
$app_url = \OC_App::getAppWebPath($app);
$app_url = null;
try {
$app_url = $this->appManager->getAppWebPath($app);
} catch (AppPathNotFoundException) {
// pass
}

foreach ($files as $jsFile) {
$this->append($root, $jsFile, $app_url);
Expand Down
16 changes: 11 additions & 5 deletions lib/private/legacy/template/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,19 @@ function emit_css_loading_tags($obj) {
* Prints a <script> tag with nonce and defer depending on config
* @param string $src the source URL, ignored when empty
* @param string $script_content the inline script content, ignored when empty
* @param string $content_type the type of the source (e.g. 'module')
*/
function emit_script_tag($src, $script_content = '') {
function emit_script_tag(string $src, string $script_content = '', string $content_type = '') {
$nonceManager = \OC::$server->get(\OC\Security\CSP\ContentSecurityPolicyNonceManager::class);

$defer_str = ' defer';
$s = '<script nonce="' . \OC::$server->getContentSecurityPolicyNonceManager()->getNonce() . '"';
$type = $content_type !== '' ? ' type="' . $content_type . '"' : '';

$s = '<script nonce="' . $nonceManager->getNonce() . '"';
if (!empty($src)) {
// emit script tag for deferred loading from $src
$s .= $defer_str.' src="' . $src .'">';
} elseif (!empty($script_content)) {
$s .= $defer_str.' src="' . $src .'"' . $type . '>';
} elseif ($script_content !== '') {
// emit script tag for inline script from $script_content without defer (see MDN)
$s .= ">\n".$script_content."\n";
} else {
Expand All @@ -96,7 +101,8 @@ function emit_script_tag($src, $script_content = '') {
*/
function emit_script_loading_tags($obj) {
foreach ($obj['jsfiles'] as $jsfile) {
emit_script_tag($jsfile, '');
$type = str_ends_with($jsfile, '.mjs') ? 'module' : '';
emit_script_tag($jsfile, '', $type);
}
if (!empty($obj['inline_ocjs'])) {
emit_script_tag('', $obj['inline_ocjs']);
Expand Down
76 changes: 64 additions & 12 deletions tests/lib/Template/JSResourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use OC\SystemConfig;
use OC\Template\JSCombiner;
use OC\Template\JSResourceLocator;
use OCP\App\IAppManager;
use OCP\Files\IAppData;
use OCP\ICacheFactory;
use OCP\IURLGenerator;
Expand All @@ -42,6 +43,8 @@ class JSResourceLocatorTest extends \Test\TestCase {
protected $cacheFactory;
/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
protected $logger;
/** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */
protected $appManager;

protected function setUp(): void {
parent::setUp();
Expand All @@ -51,6 +54,7 @@ protected function setUp(): void {
$this->config = $this->createMock(SystemConfig::class);
$this->cacheFactory = $this->createMock(ICacheFactory::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->appManager = $this->createMock(IAppManager::class);
}

private function jsResourceLocator() {
Expand All @@ -63,7 +67,8 @@ private function jsResourceLocator() {
);
return new JSResourceLocator(
$this->logger,
$jsCombiner
$jsCombiner,
$this->appManager,
);
}

Expand All @@ -84,25 +89,34 @@ private function randomString() {
}

public function testFindWithAppPathSymlink() {
$appName = 'test-js-app';

// First create new apps path, and a symlink to it
$apps_dirname = $this->randomString();
$new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname;
$new_apps_path_symlink = $new_apps_path . '_link';
mkdir($new_apps_path);
symlink($apps_dirname, $new_apps_path_symlink);
$this->assertTrue((
mkdir($new_apps_path) && symlink($apps_dirname, $new_apps_path_symlink)
), 'Setup of apps path failed');

// Create an app within that path
mkdir($new_apps_path . '/' . 'test-js-app');
$this->assertTrue((
mkdir($new_apps_path . '/' . $appName) && touch($new_apps_path . '/' . $appName . '/' . 'test-file.js')
), 'Setup of app within the new apps path failed');

// Use the symlink as the app path
\OC::$APPSROOTS[] = [
'path' => $new_apps_path_symlink,
'url' => '/js-apps-test',
'writable' => false,
];

$this->appManager->expects($this->once())
->method('getAppPath')
->with($appName)
->willReturn("$new_apps_path_symlink/$appName");
$this->appManager->expects($this->once())
->method('getAppWebPath')
->with($appName)
->willReturn("/js-apps-test/$appName");

// Run the tests
$locator = $this->jsResourceLocator();
$locator->find(['test-js-app/test-file']);
$locator->find(["$appName/test-file"]);

$resources = $locator->getResources();
$this->assertCount(1, $resources);
Expand All @@ -122,7 +136,45 @@ public function testFindWithAppPathSymlink() {
$this->assertEquals($expectedFile, $file);

array_pop(\OC::$APPSROOTS);
unlink($new_apps_path_symlink);
//unlink($new_apps_path_symlink);
//$this->rrmdir($new_apps_path);
}

public function testFindModuleJSWithFallback() {
// First create new apps path, and a symlink to it
$apps_dirname = $this->randomString();
$new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname;
mkdir($new_apps_path);

// Create an app within that path
mkdir("$new_apps_path/test-js-app");
touch("$new_apps_path/test-js-app/module.mjs");
touch("$new_apps_path/test-js-app/both.mjs");
touch("$new_apps_path/test-js-app/both.js");
touch("$new_apps_path/test-js-app/plain.js");

// Use the app path
$this->appManager->expects($this->any())
->method('getAppPath')
->with('test-js-app')
->willReturn("$new_apps_path/test-js-app");

$locator = $this->jsResourceLocator();
$locator->find(['test-js-app/module', 'test-js-app/both', 'test-js-app/plain']);

$resources = $locator->getResources();
$this->assertCount(3, $resources);

$expectedRoot = $new_apps_path . '/test-js-app';
$expectedWebRoot = \OC::$WEBROOT . '/js-apps-test/test-js-app';
$expectedFiles = ['module.mjs', 'both.mjs', 'plain.js'];

for ($idx = 0; $idx++; $idx < 3) {
$this->assertEquals($expectedWebRoot, $resources[$idx][1]);
$this->assertEquals($expectedFiles[$idx], $resources[$idx][2]);
}

array_pop(\OC::$APPSROOTS);
$this->rrmdir($new_apps_path);
}
}
29 changes: 29 additions & 0 deletions tests/lib/TemplateFunctionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,35 @@ public function testPrintUnescapedNormalString() {
print_unescaped($string);
}

public function testEmitScriptTagWithContent() {
$this->expectOutputRegex('/<script nonce="[^"]+">\nalert\(\)\n<\/script>\n?/');
emit_script_tag('', 'alert()');
}

public function testEmitScriptTagWithSource() {
$this->expectOutputRegex('/<script nonce=".*" defer src="some.js"><\/script>/');
emit_script_tag('some.js');
}

public function testEmitScriptTagWithModuleSource() {
$this->expectOutputRegex('/<script nonce=".*" defer src="some.mjs" type="module"><\/script>/');
emit_script_tag('some.mjs', '', 'module');
}

public function testEmitScriptLoadingTags() {
// Test mjs js and inline content
$pattern = '/src="some\.mjs"[^>]+type="module"[^>]*>.+\n'; // some.mjs with type = module
$pattern .= '<script[^>]+src="other\.js"[^>]*>.+\n'; // other.js as plain javascript
$pattern .= '<script[^>]*>\n?.*inline.*\n?<\/script>'; // inline content
$pattern .= '/'; // no flags

$this->expectOutputRegex($pattern);
emit_script_loading_tags([
'jsfiles' => ['some.mjs', 'other.js'],
'inline_ocjs' => '// inline'
]);
}

// ---------------------------------------------------------------------------
// Test relative_modified_date with dates only
// ---------------------------------------------------------------------------
Expand Down