johannschopplich / kirbyup

🆙 Official bundler for Kirby Panel plugins
https://kirbyup.getkirby.com
MIT License
51 stars 3 forks source link

Support dev server mode w/ HMR #17

Open jonaskuske opened 2 years ago

jonaskuske commented 2 years ago

Currently kirbyup only allows building the plugin, either for production or using the --watch option, with full rebuilds on change.

Not sure if it's feasible, but a dev server mode with HMR would be great, especially for Vue components!

johannschopplich commented 2 years ago

Hi Jonas,

indeed, that would be great. It's extremely complicated tho, in my opinion.

It would be easy, if we would build the Kirby Panel ourselves. As a matter of fact, you can do so yourself and build your own custom Panel bundle with your components baked in. Then you'll benefit from all of Vite's glory.

But since the Kirby Panel has already been built, we execute a bundled and almost "locked" Vue instance. The only entrypoint is Kirby's plugin system. Kirby handles the JS injection. In order to provide HMR, we would have to create a dev server and build a static index.js for Kirby to read anyway. Is partly HMR for a component inside a Vue bundle even possible? My only solution at the moment would be to reload the page once chances are made to your components.

Honestly, I don't know how to pull that off. Not even considering how to handle the state inside those components and save them between hot updates.

Feel free to give it a shot! If you create a POC, I'd be happy to help integrating. For now, the use case and feature request exceeds my time and use case.

Thanks for the input.

jonaskuske commented 2 years ago

For now, the use case and feature request exceeds my time and use case.

Yeah, totally get that! :)

I explored this a little and you're right, without some changes to Kirby it doesn't really work. If a plugin is served by Vite it is served as native JS modules. Kirby needs the plugin to be executed before the panel's main index.js loads, however modules execute asynchronously so a separate <script> tag doesn't work. The plugin module(s) need to become part of the index.js's module graph so it can wait for them to load first, which is something Kirby needs to support in order to proceed. I think this might be a worthwhile and relatively non-invasive change and implemented it here: https://github.com/getkirby/kirby/commit/a23e35771dba41f1d33f046f52b02b447e8b35bc

With that in place, we can use a plugin entry file that "relays" the request to a Vite development server started by kirbyup:

// plugins/kirby-example-plugin/index.mjs

import 'http://localhost:5173/src/index.js'

Now we got working HMR! Almost. For CSS changes it works, which is quite nice on its own, and it also works for <template> changes. (those swap the render function and force a re-render, but keep the component instance) But when the <script> part of a component is edited, the entire component is reloaded, which can cause errors. Kirby manipulates the options of plugin components "from the outside": https://github.com/getkirby/kirby/blob/main/panel/public/js/plugins.js Vite doesn't know about these manipulations, so when the HMR runtime reloads the component and switches to a newer component instance, this instance only includes the options from the component definition, not the ones Kirby patched-in later. Due to this, a section component doesn't include the section mixin: calls to this.load() throw errors.

The easiest way to prevent such errors is to enable HMR only for CSS changes and reload the page if the JS/template was changed. This is how my current HMR implementation for kirbyup works: https://github.com/jonaskuske/kirbyup/tree/feat/hmr

It should also be possible to discern JS updates that cause the HMR runtime to refresh (good) from updates that cause a component reload (bad), and only trigger a page reload for the latter. However whether to refresh or to reload is only decided at runtime, so the only way I can currently think of is patching the HMR runtime code, something like:

// page reload instead of hot component reload
__VUE_HMR_RUNTIME__.reload = () => import.meta.hot.invalidate()

Then we'd have HMR for the <template> and <style> section.

If Kirby where to change its plugin loading behavior so it doesn't manipulate component definitions, we could have full HMR. Not sure about extends, but the mixin could be registered as a global mixin, for example. But that aside, HMR just for <style> or for <style> and <template> are already worthy DX improvements on their own. kirbyup could also use something like vite-plugin-live-reload to force a page refresh when a plugin's PHP file changes, to make the HMR plugin development experience complete. :)

johannschopplich commented 2 years ago

I'm blown away by your approach, Jonas. A serious Vite pro is in the house! Amazing contribution. 👏

Will look into it next week. Looks stunning so far. Great improvements! We'll put that into the core definitely.

jonaskuske commented 2 years ago

Heh, thank you! 😊

Just committed the change so template refreshes work with HMR and only instance reloads cause a full page reload: https://github.com/jonaskuske/kirbyup/commit/fcf6fdeec47a805c411b69ec4fd5ebd5369c65a9


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


Documenting outstanding questions and todos here so they can be discussed later:

Kirby Core

I've submitted https://github.com/getkirby/kirby/pull/4541 in the Kirby repo to discuss Kirby-related changes there. :)

Kirbyup

jonaskuske commented 2 years ago

The required changes to Kirby are merged and slated for release in 3.7.4 🚀🚀

I'll be on holiday soon and not sure if I'll manage to write the tests that will complete the PR before that, but either way this feature should be live fairly soon! :)

johannschopplich commented 2 years ago

Nice! I think the PR is pretty solid and extensively tested by you. Even tho we don't have automated tests yet, I intend to release a beta of the new kirbyup version (2.0) for user's to test and catch up with tests later. Would you be OK with that approach?

I'm gonna be on vacation soon as well. Perfect timing.

johannschopplich commented 2 years ago

Reopening since automatically closed by GitHub. Automated tests are still missing.

jonaskuske commented 2 years ago

Heh, and the fix for the Vue SFC compiler has landed as well: https://github.com/vuejs/vue/pull/12732#event-7185567352! 🎉

And yep, definitely okay with that approach! I don't think it's a breaking change though, npx kirbyup <src> and npx kirbyup <src> --watch should behave exactly the same. So this would also be fit for a 1.4 feature release, unless I'm missing something?




As far as actual breaking changes go, I was thinking about re-evaluating the need for the thin abstraction over Vite vs. using Vite's own config resolving mechanism (and CLI commands?) directly. Config with custom option could look like:

// vite.config.js
export default defineConfig({
  resolve: { alias: { insteadOf: 'kirbyup.config.js#alias' }},
  plugins: [kirbyup({ customOption: true })]
})

Could simplify the code base and lead to less code to maintain.

As for CLI commands, we could either

But those are just some ideas that came to my mind while working on this PR, also inspired by the changes Svelte Kit went through – might be unnecessary, and even if worth considering, definitely material for a separate issue/PR.