Skip to content
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.

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

Allow apps to include module js / ES6 scripts #36057

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 10, 2023

Summary

Allow apps to provide scripts as ES6 modules by adding a type="module" to script tags of module files.
This is done by changing JSResourceLocator to also look for .mjs files, and fallback to .js (this allow app developers to provide both files so their apps can be used even on Nextcloud version not supported this feature while using the modern scripts on new Nextcloud versions).

The benefit of this is apps are no longer required to rely on module loading from webpack & co but can utilize the module loading of the browser (fully supported by all of our browser targets). Moreover this allows to create much smaller bundles.
Example:

Below is an example for reduced bundle size of the `forms` app (selected because mid-sized app)
method with source maps just code
Currently using webpack for bundling / chunking 26580 KiB 7332 KiB
ES6 browser modules (compiled using vite) 11748 KiB 4068 KiB

But as said it is even possible to now to quit using a bundler at all and let the browser load needed modules,
as something like this is possible:

// my-app/js/entry.mjs
import { someFunction } from './other.mjs'

someFunction();
// my-app/js/other.mjs
export function someFunction() { /* */}

Other considered solutions

I also considered adding a flag on OC\Util::addScript (e.g. an isModule parameter).
But this would require to rewrite / change a lot of internal utilities as this information must be carried along through
the OC\Util, the TemplateLayout (which also would have to work with deprecated OC_Util scripts), the JSResourceLocator and then be printed by the template functions.

TODO

  • Add documentation to the manuals
  • Add support within docker image (mime info and rewrite rules in .htaccess)

Checklist

@susnux susnux added the 3. to review Waiting for reviews label Jan 10, 2023
* @see appendIfExist()
*/
protected function appendScriptIfExist($root, $file, $webRoot = null) {
if (!$this->appendIfExist($root, $file . '.mjs', $webRoot)) {
Copy link
Member

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

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).

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

nice addition!

see comments

lib/private/legacy/template/functions.php Outdated Show resolved Hide resolved
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I do not really understand what this does given my low js knowledge, but left a few comments regarding PHP code quality.

lib/private/Template/JSResourceLocator.php Outdated Show resolved Hide resolved
lib/private/Template/JSResourceLocator.php Outdated Show resolved Hide resolved
lib/private/Template/JSResourceLocator.php Outdated Show resolved Hide resolved
lib/private/Template/JSResourceLocator.php Outdated Show resolved Hide resolved
lib/private/legacy/template/functions.php Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feat/esm-js-scripts branch 4 times, most recently from 550da80 to e5134f3 Compare January 10, 2023 10:56
@susnux susnux requested a review from PVince81 January 12, 2023 10:00
@susnux susnux force-pushed the feat/esm-js-scripts branch from e5134f3 to 5747a35 Compare January 22, 2023 13:12
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Nice 👍

@max-nextcloud
Copy link
Contributor

@susnux This looks good. Do you have an example of how this would be used? I'd like to give it a try to understand it better.

@susnux
Copy link
Contributor Author

susnux commented Jan 30, 2023

This looks good. Do you have an example of how this would be used? I'd like to give it a try to understand it better.

@max-nextcloud
A simple example you have app-main.mjs and app-settings.mjs sharing the same code, e.g. a custom vue component, this would allow you to write the files like this without webpack:

  • app-main:
import MyComponent from './my-component.js'
// ...
  • app-settings:
import MyComponent from './my-component.js'
// ...
  • my-component.js
export default {
// vue stuff
}

But a more exciting thing this would allow us: Using vite instead of webpack, as vite uses ESM for module loading.
E.g. I tested this with the forms app, see first comment, you can test this yourself:

  1. Setup a nextcloud server with this PR
  2. Setup your apache to support mjs as javascript: edit .htaccess
  3. Change rewrite condition to look like this (added mjs): RewriteCond %{REQUEST_FILENAME} !\.(css|js|mjs|svg|gif|png|html|ttf|woff2?|ico|jpg|jpeg|map|webm|mp4|mp3|ogg|wav|wasm|tflite)$
  4. Add mime type (see the mod_mime section of the file): AddType text/javascript mjs
  5. Clone and build the forms app, use the feat/vite branch which I created for demonstration purpose

For me the difference between webpack (ES5) and vite (ES6) is huge:

size (bytes) time (ms)
ESM (vite) 13440689 10'419
ES5 (webpack) 28907503 22'135

Moreover this could be used later for other things, like e.g. sharing modules, I can image having a nextcloud wide importmap which provides aliases for common modules, like @nextcloud/... or vue, so that every other app can declare that dependency as external and the browser uses this shared dependency (which of cause benefits from the browser caching so that loading times and transfer size of the overall nextcloud instance would drop significantly.

@susnux susnux force-pushed the feat/esm-js-scripts branch 2 times, most recently from 057e3d8 to 6577ea8 Compare February 22, 2023 20:16
…Locator`

Move from `\OC_App::getAppPath` to `IAppManager::getAppPath`.

Signed-off-by: Ferdinand Thiessen <[email protected]>
Enable module js (ES6) support on the `JSResourceLocator`.
This changes `JSResourceLocator` to look for `.mjs` files first
to allow applications to provide a fallback `.js` for older Nextcloud versions.

Signed-off-by: Ferdinand Thiessen <[email protected]>
If apps are installed in non standard app paths, we need
to check `$app_path/$script` instead of only doing so for translations.
Without this it would fallback to `.js` extension even if a `.mjs` file exists.

Also tried make the code more selfe explaining.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the feat/esm-js-scripts branch from 6577ea8 to a3595f7 Compare February 22, 2023 20:19
@susnux
Copy link
Contributor Author

susnux commented Feb 22, 2023

I rebased this, CI is happy. Anything else I can help with to get this further? :)

@max-nextcloud
Copy link
Contributor

max-nextcloud commented Feb 23, 2023

@susnux This sounds super exciting on so many levels. I have not had time to play with it yet but plan to do so in the next days. I had a look at the code and see that the comments by @come-nc were addressed and it looks good to me.

I'd love to see this in asap - and we're really short before releasing Nextcloud 26. So I am wondering if we should try to get this in or not.

Based on your instructions I am mostly thinking about the following scenario:

  • We include this in NC 26.
  • In a few months apps start using it as you described in versions also released for 26.
  • Some admin deployed NC 26 with one of those apps installed without making the required changes to .htaccess.

My gut feeling says Nextcloud would look for *.mjs files and find them and therefore add <script type="module"> tags for them. However these modules could not be retrieved because apache would fail to deliver them.

We'd obviously need to document the necessary changes to .htaccess. But documentation might not be enough. I'm afraid many people do not read update instructions.

I also don't know if postponing this merge to land after the branch off of stable26 would help with this. It would buy us time to document and test things better - but at the same time it would mean we could not use mjs in Nextcloud 26.

@juliushaertl, @skjnldsv, @nickvergessen You're more familiar with release processes and ways to handle such things. What's your take on this?

@nickvergessen
Copy link
Member

We'd obviously need to document the necessary changes to .htaccess. But documentation might not be enough. I'm afraid many people do not read update instructions.

.htaccess is updated during updates with a repairstep. But that also means the nginx docs need adjusting, etc.

You're more familiar with release processes and ways to handle such things. What's your take on this?

Wait 1 more week until stable26 is branches off and then we can merge this as the first 27 feature.

* 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

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

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.

@skjnldsv
Copy link
Member

This is too late for nc26.
We can merge now for 27 and that should let us find some time to find bugs

@skjnldsv
Copy link
Member

5. Clone and build the forms app, use the feat/vite branch which I created for demonstration purpose

Time for a @nextcloud/vite-vue-config repository! 😁

@susnux
Copy link
Contributor Author

susnux commented Mar 16, 2023

@nickvergessen

.htaccess is updated during updates with a repairstep

Is this generated from the .htaccess inside the root of this repository? Then I can adjust it. See the latest commit.

But that also means the nginx docs need adjusting, etc.

Waiting documentation PR: nextcloud/documentation#9909

@juliusknorr juliusknorr merged commit 8b4d49c into master Mar 22, 2023
@juliusknorr juliusknorr deleted the feat/esm-js-scripts branch March 22, 2023 08:17
@ernolf
Copy link
Contributor

ernolf commented May 5, 2023

@susnux
4. Add mime type (see the mod_mime section of the file): AddType text/javascript mjs

@skjnldsv approved these changes on Mar 16
@susnux
Adjust .htaccess to serve .mjs files with javascript mime type

short version:
The correct mimetype for .js files is application/javascript and not text/javascript
The correct mimetype for .mjs files is application/javascript-module and not text/javascript either.

detailed:
The problem with using the wrong MIME type is that it may cause unexpected behavior in some clients or servers. While some browsers may still execute the JavaScript code correctly, others may not be able to parse the code or may treat it as plain text, which could lead to errors or security vulnerabilities.

Additionally, some servers or caching systems may use the MIME type to determine how to handle the content. For example, a server may compress content with gzip if the MIME type is "application/javascript", but not if the MIME type is "text/javascript". Using the wrong MIME type could therefore affect the performance.

When a browser fetches an ECMAScript module with the type="module" attribute, it will treat it as a module and perform some additional processing, such as loading the module dependencies. The application/javascript-module MIME type also allows you to use the import and export statements to create and consume modules.

So, using the application/javascript-module MIME type for .mjs files can help ensure that the browser processes them as ECMAScript modules and enable the full benefits of this technology.

I have created this post from my general knowledge, unaware of any other reasons that may lie in the code of the server that I am not aware of. If that's the case, I apologize

@susnux
Copy link
Contributor Author

susnux commented May 5, 2023

Hey :)

short version: The correct mimetype for .js files is application/javascript and not text/javascript The correct mimetype for .mjs files is application/javascript-module and not text/javascript either.

This is not correct, as per IETF RFC 9239 text/javascript is the standard and application/javascript is considered obsolete (also MDN best practices and also the only supported MIME type as of the HTML specification).
Also text/javascript is supported by all browsers, whereas application/javascript-module is not and will case scripts to being not loaded.

With which servers did you experienced that issues (text/javascript vs application/javascript)? Because this should probably reported there as a bug.

@ernolf
Copy link
Contributor

ernolf commented May 5, 2023

As I see now, the /etc/mime.types in debianoid systems has also been updated.

That's exactly what I feared, that I might be building on old imperfect information after all. Hence my caveat in the last paragraph
Thank you for the update.

@ernolf
Copy link
Contributor

ernolf commented May 9, 2023

@susnux:

This is not correct, as per IETF RFC 9239 text/javascript is the standard and application/javascript is considered obsolete (also MDN best practices and also the only supported MIME type as of the HTML specification).

I don't know where to report this, but since you seem to be the expert, shouldn't this be updated in the documentation as well?

https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html?highlight=application/javascript

@susnux
Copy link
Contributor Author

susnux commented May 11, 2023

@ernolf Good point I will file a PR to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants