Allow panel plugins to load index.dev.mjs instead of index.js (needed by kirbyup)

[jonaskuske]

This PR allows plugins to use a module-based index.dev.mjs as entry

Example repo to try it out: https://github.com/jonaskuske/kirbyup-hmr-playground

I'm currently doing work so Kirby plugins developed with kirbyup can use Vite features like HMR during development, which much improves the DX of authoring panel plugins that include Vue components: johannschopplich/kirbyup#17 (comment)

However there are some constraints: Kirby plugins need to load in order (have to be registered on window.panel.plugins before the panel initializes), and the Vite dev server works with native JS modules, which execute asynchronously – so when panel and plugin are loaded separately, in-order execution can't be ensured.

To work around that, Kirby would either need to support the async registration of plugins (which I expect to be quite the invasive and complex change), or include module-based plugins in the panel's module graph via an import to make sure they execute first, which is what this PR implements.

Implementation details

Instead of loading the panel code directly via <script type="module" src="path/to/panel.js">, it is now loaded like this:

<script type="module">
import('path/to/panel.js')
</script>

This allows us to inject imports for module-based plugins before the panel code is imported:

<script type="module">
import 'plugin1/index.dev.mjs';
import 'plugin2/index.dev.mjs';

import('path/to/panel.js')
</script>

Note that the import of panel.js must be a dynamic import(): in-order-execution of static imports is only guaranteed for synchronous modules, so if the first import uses top level await, the second import can actually run before the first one.

Loading the panel code through a dynamic import as part of the module body makes sure the panel runs after all plugins are loaded, since a module body executes after all static imports are done.
(in browsers supporting link[rel=modulepreload] the panel code is still fetched and compiled ahead of time)

This leaves us with one last problem: a single failing plugin import can prevent the entire panel from loading as there's no way to catch errors from a static import. To solve this, plugin modules themselves are loaded through a dynamic import, which can be wrapped in try/catch:

<script type="module">
try {
  await Promise.all(['plugin1/index.dev.mjs', 'plugin2/index.dev.mjs'].map(uri => import(uri)))
} catch {
  // some plugins errored but we'll still continue to load path/to/panel.js!
}

import('path/to/panel.js')
</script>

As a perf enhancement and to avoid having to copy index.dev.mjs to /media and then delete it again when the kirbyup development server is stopped and the index.dev.js is removed, the plugins are loaded inline, as data URIs:

<script type="module">
try {
  await Promise.all(
    [
    'data:text/javascript;base64,aDfghddplugin1==',
    'data:text/javascript;base64,aDfghddplugin2=='
    ].map(uri => import(uri))
  )
} catch {
  // some plugins errored but we'll still continue to load path/to/panel.js!
}

import('path/to/panel.js')
</script>

If a plugin has an index.dev.mjs which is loaded through the technique above, its regular index.js will be excluded from the plugins/index.js bundle. Otherwise, its index.js is loaded as part of plugins/index.js as it used to.

Breaking changes

No, the only change is that panel.js is now loaded via a dynamic import(), which is covered by Kirby's minimum browser requirements. All other changes only have an effect if a kirbyup development server is running and created an index.dev.mjs.

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

• Add changes to release notes draft in Notion
• Add to website docs release checklist (if needed)

[lukasbestle]

Thank you very much for your contribution. I like the concept as it really feels like something that can just work™ without any need to think about the details as a user. Especially the idea to dynamically generate the index.mjs file in kirbyup and remove it again when kirbyup is stopped is really great.

A few comments:

• Currently you dynamically inject the import into the Panel JS in a route. This will break when a CDN is used to serve the media folder. Also it's not very efficient to spin up PHP for every Panel JS request. We need a way to inject the import at build time so that the media folder can be served statically.
• Requesting files directly from site/plugins will not work with the default and recommended configuration. The site directory must not be directly accessible to avoid security breaches.
• I think this should be a dev-only feature. In production it adds additional complexity and is probably not as reliable as a single bundle that can again be served via a CDN if needed. So I'd be a fine with a configuration option that can then be enabled in a domain-specific config file just for the dev domain.

[jonaskuske]

Also it's not very efficient to spin up PHP for every Panel JS request. We need a way to inject the import at build time so that the media folder can be served statically.

Yeah, agree that this isn't optimal! But since the media URL can vary, I didn't see how it can be injected at panel build time, which happens during release and before we know anything about a user's configuration.

Another way would be to load both the plugin and panel JS from an inline module:

<script type="module">
import '/media/plugins/index.mjs'

import('/media/panel/assets/js/index.js')
</script>

Not sure if the browser's preload scanner detects these imports AOT, if yes it shouldn't make a difference perf-wise, otherwise we'd also need to add a <link rel="modulepreload"> to the head. Which is a good idea anyway.

I think this should be a dev-only feature.

Makes sense! I think .mjs support in shipped plugins would be cool for "forward-thinking, we support the latest and greatest" reasons, but you're right, not really for practical ones. Dev-only avoids a lot of complexity, and especially...

Requesting files directly from site/plugins will not work with the default and recommended configuration.

...if we know the index.mjs is just for development and only ever includes an absolute import that goes to Vite, we can move it to the media folder without causing issues for relative imports. (because there are none)
I think we could even skip loading the actual .mjs files all together and embed them inline as data: URLs – from my testing, base64-encoded inline modules that include relative (or just not-fully-qualified) URL imports fail silently, but full URL imports (and inline code) work without issues.

Gonna push some changes later today. :)

Edit:
I also think we can make do without a config option, which would mean it just works ™️ even better. If plugins have .mjs files, include them via top level await, otherwise don't. Maybe change the name to _index.mjs to make it clear this is an internal, dev-only feature. Then document the browser version requirements in the docs for kirbyup serve.

[jonaskuske]

Updated the loading behavior as described in my comment above. Let me know what you think!

(the plugins.mjs is always loaded so we don't have to read its contents before adding the script, but it's empty when there are no _index.mjs files – so there's no top level await in the source unless a plugin Vite server is running, the browser requirements stay the same)

[distantnative]

I added a few comments. Would be great to have more inline comments as well to understand why we are doing these extra jumps etc. also 2,3 years down the road.

[jonaskuske]

@lukasbestle
Regarding the failing unit test:

I noticed that behavior before: if a JS file contains less than 3 chars, the reported mime type isn't text/javascript but application/octet-stream. Would you rather have me:

a) fix the test (use ccc instead of c as module contents?)
b) fix the code, by adding another entry to the fixing map in Mime.php:

'application/octet-stream' => [
  'mjs' => 'text/javascript'
]

(option b) is currently implemented)

[lukasbestle]

I think it's great.

Just three things still to do IMO:

• Could you please update the first post of this PR with the current info?
• Another unit test that covers this case (no index.dev.mjs) would be great
• Open discussion about where to place the filter code; @distantnative, what do you think?

[jonaskuske]

@lukasbestle All done!

And another thing: just realized that this could actually be implemented without top level await, by using an async IIFE in the main <script type=module> and exporting promises from the index.dev.mjs files, which the main script can await before importing panel.js. This would lower the browser version support requirements, but with this being dev-only and all current browsers supporting top level await, I don't think it's worth the change. Opinions?

[lukasbestle]

This would lower the browser version support requirements, but with this being dev-only and all current browsers supporting top level await, I don't think it's worth the change.

Fully agree!

[lukasbestle]

Mostly coding style stuff, the rest looks really great. Thanks a lot for the extra code comments. 👍