getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.31k stars 168 forks source link

Plugins break if they use Vue 2.7 functionality as the Panel doesn't expose the required functions #4735

Closed jonaskuske closed 1 year ago

jonaskuske commented 2 years ago

Description

Panel plugins are built with Vue, but don't ship their own runtime. Instead, they're loaded into the Vue instance the Panel app itself is using. Because of this, Vue should be externalized in the build config of kirbyup or a respective manual plugin setup, so your component is then compiled to code similar to this:

(function(_vue) {
  return {
    name: 'MyKirbyPanelComponent',
    data: () => ({ level: 50 }),
    created() {
      console.log('Panel component ready!')
    },
    methods: {
      inc() { this.level += 10 }
    }
    template: `
    <div>
      <p>Kirby is this awesome: <b>{{ level }}/100</b></p>
      <button @click="inc">Increase awesomeness</button>
    </div>`
  }
})(Vue) /* pass in the global window.Vue instead of shipping your own instance of "vue" */

Unfortunately, while Kirby does provide access to the global Vue instance via the use option, it is not exposed globally as window.Vue, window.panel.Vue2 or similar, so in the example above, Vue at the bottom is actually undefined and breaks the plugin if used. This has caused some issues in the past, e.g. https://github.com/johannschopplich/kirbyup/issues/8 or https://github.com/johannschopplich/kirbyup/issues/24 (where workarounds could be implemented, e.g. by using a separate library instead of Vue for the event bus)

If some compiled components need access, Kirby plugins can work around that through the use option and expose it themselves, using plugin code like this:

panel.plugin('my/example', {
  use: {
    plugin(Vue) {
     window._Vue = Vue
    }
  }
})

Now Vue 2.7 introduced new functionality for which access to the Vue package is required, but due to module shenanigans the plugin workaround from above is not sufficient anymore: The new functionality is implemented using functions that are independent from the main Vue constructor, like useCssVars() or onMounted(). In the universal UMD build of Vue, those functions are attached to Vue, so can be accessed through e.g., Vue.useCssVars. However, in the ES module build, those functions are (only) exported separately:

export default Vue

export function useCssVars() {}

In that case, the new 2.7 functions can not be accessed via Vue.useCssVars. Since Kirby bundles this ES module version, the Vue constructor it exposes via the use option does not have the new functions attached and thus 2.7 functionality is not usable at all, even if a plugin opts-in by using the use option to give its components access to Vue.

Here's an overview of Vue 2.7 features and changes that all rely on Vue to be there and break otherwise:

Most prominent, support for the Composition API With the Composition API, components can now use `setup` and ` ``` The compiled code will try to access `Vue.useCssVars`.
JSX Render functions compiled from JSX now use the global `h` export instead of a parameter: ```js // 2.7 render() { return _vue.h('h1', 'Hello World') } // <= 2.6 render(_h) { return _h('h1', 'Hello World') } ```

So like before, by default Panel plugins break if the plugins tries to use Vue or any of its methods. Now with version 2.7 and its support for the Composition API (setup/<script setup>), that is both much more likely to happen and can't be worked around anymore through use. So instead of your component you'll see:

Uncaught ReferenceError: Vue is not defined
    at index.js?1664631505:3:1174

// or, if Vue is exposed:

Uncaught TypeError: Vue.useCssVars is not a function
    at index.js?1664631505:3:1174

😢

Expected behavior

Either:

or:

To reproduce

https://github.com/jonaskuske/kirby-vue-2-7-playground

Your setup

Kirby Version

3.7

Console output

Uncaught ReferenceError: Vue is not defined
    at index.js?1664631505:3:1174

Your system (please complete the following information)

Additional context

Suggestions for fixing this (happy to submit a PR!):

Or:

lukasbestle commented 2 years ago

Thank you very much for the detailed and well-structured information. 💛

I think we should certainly support sharing of the Vue object between the core and plugins. It doesn't make sense to limit the functionality if there's a better way and it makes even less sense that each plugin ships its own Vue instance.

What's the most solid and future-proof solution in your opinion? IMO externalizing window.Vue sounds good as it's easy to understand and doesn't require any magic. It would also be easy to implement if I understood it correctly.

distantnative commented 2 years ago

To be honest, I don't think there is a healthy way to realise this with Vue 2 and its nature of importing the Vue instance in various places. Sharing this with plugins in more ways than currently done can break many things and would be quite some efforts I doubt right now are really beneficial compared to the amount of work - from how I perceive the work right now.

All plugins still work fine as they have with Vue 2.5, 2.6... Vue 2.7 indeed started to back port some Vue 3 functionality that does not work right now because of the way we handle it. But I think the only way forward is working more and more on a path to use Vue 3. I honestly think that even if we would expose the Vue instance somehow even with Vue 2.7, you'd see other parts where those attempts break as the Panel was designed for Vue 2 - so the very different Vue 3 stuff isn't really a fit so far.

distantnative commented 2 years ago

I'm not even sure if this should stay as an issue or is rather an idea for https://kirby.nolt.io tbh.

bastianallgeier commented 2 years ago

Don't we already provide ways to access the Vue instance? We have the created hook for plugins which gets access and we also put Vue in window.panel.$vue

distantnative commented 2 years ago

No, we put the app instance in window.panel.$vue/window.panel.app but not Vue.

bastianallgeier commented 2 years ago

Ah right. Sorry for mixing this up.

jonaskuske commented 2 years ago

makes even less sense that each plugin ships its own Vue instance

I don't think that's even an option, as Vue tracks internal state like currentInstance which would probably not work with multiple Vue apps running at the same time.

What's the most solid and future-proof solution in your opinion?

Externalizing Vue is pretty solid and should be future-proof imo – like you said, globally exposed UMD modules aren't any kind of magic but an established pattern since forever and well supported by virtually all build tools. But maybe exposing it on window.panel.Vue (or window.panel.Vue2) instead of directly on window would be worth considering? That way we wouldn't introduce another magic global variable that can potentially conflict with other code, browser extensions, etc. and the whole "public interface" would remain neatly namespaced under window.panel as it is right now.

can break many things and would be quite some efforts

Which kinds of breakage do you expect? I can't think of any off the top of my head, but I don't know the intricacies of Vue usage inside the Panel app and the history of past plugin issues like you probably do, so definitely can't make authoritative statements on this!

Anyway, I'm gonna throw a little example together in addition to the repro so we can play around with it and see how it works :)

I'm not even sure if this should stay as an issue or is rather an idea for kirby.nolt.io tbh

I think that even if feature support is a wontfix, this is still an issue, a documentation issue in that case. Kirby ships Vue 2.7, allows plugins to use Vue, but breaks without any explanation or documentation if a component uses something like v-bind in CSS. I don't know how that can be expected behavior, it must definitely be documented imo. Features like v-bind in CSS, JSX or the currently experimental reactivityTransform add imports to Vue automatically during compilation without the user knowing about it, so imo we'd have to explicitly point out the unsupported features, alongside a more general rule like "never write imports that reference vue and never use dependencies that import it" or something like that.

jonaskuske commented 2 years ago

if we would expose the Vue instance somehow even with Vue 2.7, you'd see other parts where those attempts break as the Panel was designed for Vue 2

Hmm, but Vue 2.7 is a feature release and doesn't break apps designed for Vue 2? You are already running it in production 🤔

Still, fully agree that if v2.7 support for plugins turns out to require more than a small config change, it's probably not worth it and documenting the limitations is a better way forward.

The path to Vue 3 is going to be quite the endeavor though 😅

lukasbestle commented 2 years ago

We will certainly need a solution for this also for Vue 3, won't we? So I feel like solving this already with Vue 2.7 will make the migration to Vue 3 easier.

distantnative commented 2 years ago

Vue 3 handles this very differently. There it's all about the App instance and not about a global Vue object that gets imported in all kinds of files.

And due to the weird nature of Vue in 2.x I'm actually worried if stuff won't break if we expose it to plugins that can directly add a plug-in, interact with the store at points that aren't quite defined in the lifecycle.

I think it could also make the transition to Vue 3 even harder - it would be another big breaking change then likely.

distantnative commented 2 years ago

And considering our resources and priorities, I think we should put any Vue refactoring effort into a path to Vue 3. We really will need it eventually.

lukasbestle commented 2 years ago

OK, if Vue 3 doesn't need this and if this even hinders the migration, I completely agree that we shouldn't do it.

I guess we should document then that the backported features from Vue 3 are not yet available in Kirby.

jonaskuske commented 2 years ago

We will certainly need a solution for this also for Vue 3, won't we? So I feel like solving this already with Vue 2.7 will make the migration to Vue 3 easier.

Yes, Vue 3 absolutely needs this, way more than Vue 2.7. This is what the compiled HelloWorld.vue component (from the default Vue project that create-vite bootstraps, it includes a counter, rendering a prop and a couple links) looks like for Vue 3 — it calls vue.something() for basically everything it does:

image

(collapsed my response re security/breakage, probably irrelevant with Kirby already allowing access to global Vue, see my next comment) But yeah, in Vue 2, the Vue module includes both the global functions (that 2.7 functionality and Vue 3 is based on) *and* methods that can affect the app instance, like `Vue.component` or `Vue.use`. So while Vue 3 *definitely* needs a solution to share the Vue module with the plugins like the one I proposed, that wouldn't give plugins access to these global methods, as with Vue 3, they're found on the app instance. With Vue 2, they'd technically be accessible, so a plugin *could* try to register a global component, directive etc. that conflicts with an already existing one. I don't think that's very likely though, and also: - the plugin documentation should state that plugins must not use `Vue.{use,mixin,component,filter,directive}` - these methods have component level equivalents, so incentive to use the global ones is low to begin with - we could probably enforce method restrictions with code like this: ```js var restrictedMethods = ['use', 'mixin', 'component', 'filter', 'directive'] for (const method of restrictedMethods) { const originalMethod = Vue[method] Vue[method] = function() { if (!calledByPanel()) { throw new Error('Plugins must not use global Vue methods!') } originalMethod.apply(Vue, arguments) } } ```
jonaskuske commented 2 years ago

Okay wow, I just realized that actually @bastianallgeier was right here:

Don't we already provide ways to access the Vue instance? We have the created hook for plugins which gets access and we also put Vue in window.panel.$vue

While both $vue and created only have access to the instance, Kirby plugins can also register Vue plugins under use. The whole point of plugins is to "add global-level functionality" and as such they receive the root Vue object itself. So this is already supported:

I'm actually worried if stuff won't break if we expose it to plugins that can directly add a plug-in

And indeed, it works just fine:

window.panel.plugin('my/plugin', {
  use: {
    plugin(Vue) {
      Vue.prototype.$api = 'overriding Kirby stuff'
      Vue.use(MyOwnPlugin)
      Vue.mixin({
        created() {
          console.log(`Messing with the internals of ${this.$options._componentTag}!`)
        }
      })
    }
  }
})

I can also do:

use: {
  plugin(Vue) {
    window.Vue = Vue
  }
}

...which almost gets 2.7 functionality to work without changes to Kirby, but unfortunately Kirby doesn't ship the complete Vue module. The UMD browser build has both global methods like .mixin() and exported functions like onMounted exposed on Vue, but Kirby ships a bundled ES Module build of Vue. There, the 2.7 functions are not attached to Vue but separate named exports. So while I can access Vue.mixin, Vue.compile etc., Vue.useCssVars is not available.

So that changes my request, suggested bugfix, whatever fits best: please ship the UMD build that includes all Vue functionality instead of the current ESM one. The current behavior is basically Exposing the Vue module: the bad parts :( Whether Kirby attaches it somewhere globally or a plugin that wants to use it has to do that manually (can even be a namespace it chooses, e.g. window._kirbySeoPlugin.Vue = Vue) isn't as important.

I've updated the original post accordingly.

And btw, I've created the example repo here: https://github.com/jonaskuske/kirby-vue-2-7-playground These are the changes I made to allow 2.7 usage, really not that much: https://github.com/jonaskuske/kirby-vue-2-7-playground/commit/c90cd45e78d6952f60ecd8a2cc70e05c6cb54c23 (though the method of attaching the CDN version of Vue to window.panel isn't quite production-ready 😄)

jonaskuske commented 1 year ago

Fixed in Kirby 3.9.5