-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(workflowengine): require a web component as operation plugin #50783
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
Conversation
|
/backport to stable31 |
|
/backport to stable30 |
|
Might need to do the same stuff for registering |
275ddb9 to
9ac3a96
Compare
| const View = this.operation.component | ||
| this.component = new View() | ||
| this.component.$mount(this.$refs.operationComponent) | ||
| this.component.$on('input', this.updateOperation) | ||
| this.component.$props.value = this.rule.operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, such an approach only works in Vue 2 (which is EOL since 31 Dec 2023).
In the current Vue:
- Component cannot be used as a constructor with
new component(). Only app can, created withcreateApp(component). And thiscreateAppmust be from the sameVue, ascomponent= same issue as in the past $onwas deprecated and has been removed
Let me think about a simple alternative...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 1: WebComponents
// In an app defining operation
import Vue from 'vue'
import wrap from '@vue/web-component-wrapper'
// Wrap Vue component into custom HTML Element
const CustomOperationElement = wrap(Vue, OperationComponent)
// Register with some UNIQ name
window.customElements.define('custom-operation', CustomElement)
// In Vue 2, wrap doesn's support disabling shadow :(
// Disable with a hack
Object.defineProperty(CustomElement.prototype, 'attachShadow', { value() { return this } })
Object.defineProperty(CustomElement.prototype, 'shadowRoot', { get() { return this } })
// ---
// In workflowengine
// <component :is="'custom-operation'" />There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 2: provide a register method so CustomOperation developers can manually mount the app.
We do it in Viewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @susnux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the webcomponent approach, but we should be consistent and either also do so on viewer or use option 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr I mean for the next version nextcloud/viewer#2395
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both - but I agree the Webcomponent looks more elegant.
What would the event listener in the workflow engine look like? Could we do something like:
<component :is="'custom-operation'" @input="handleInput" />
or even use v-model as we currently do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vue/web-component-wrapper docs say:
Custom events emitted on the inner Vue component are dispatched on the custom element as a CustomEvent. Additional arguments passed to $emit will be exposed as an Array as event.detail.
Sounds like event handing should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only works in Vue 2 (which is EOL since 31 Dec 2023).
Just to clarify. Even if the server isn't about to be migrated to Vue 3 very soon, this limitation won't allow other apps to use Vue 3 and have integration with the workflow engine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both - but I agree the Webcomponent looks more elegant. What would the event listener in the workflow engine look like? Could we do something like:
<component :is="'custom-operation'" @input="handleInput" />or even use
v-modelas we currently do?
Sounds like event handing should work.
Yeah, it's a little bit more complicated. Events are emitted as native custom events. So:
- You don't have the input value as the event object, but a CustomEvent object. The value is
$event.detail[0] - You cannot use
v-model(for the reason above) - You cannot use
inputas the custom event name, until you stop bubbling nativeinputevent
So I'd suggest to use modelValue + update:modelValue event to not conflict with natives.
<component
:is="operationElementName"
:model-value="inputValue"
@update:model-value="inputValue = $event.detail[0]"
/>|
Thank you all for the input here! I pushed the suggested changes hereto as well as to Talk's flow. I could not get the Also, the But I might have been holding it wrong, this is far from my area of confidence 😅 Didn't do yet:
|
|
@blizzz I'll have a look in the morning. I must work, I checked :D As an alternative, we can also define both options:
Then we have a quick and simple fix for the release + a better solution for the future |
if the app has to be touched anyway – it has – there is no reason to have a bridge solution there (provided WebComponents works). less is more. |
max-nextcloud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not take a look at the entire file - just some had a brief glance. Hope the pointers are useful. Happy to have a call to follow up.
3a2794a to
33a8223
Compare
33a8223 to
d508de1
Compare
|
Updated, if I recall correctly @max-nextcloud, for a minimal fix we want to stay like this. I still need to
|
|
/backport to stable30 |
Signed-off-by: Arthur Schiwon <[email protected]>
|
Failing test not related. |
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]>
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Carl Schwan <[email protected]> [skip ci]
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
* Based on nextcloud/server#50783 * For #514 * Server companion: nextcloud/server#54788 * Library companion: nextcloud-libraries/nextcloud-sharing#95 We need to provide an API that does not rely on Vue. As Vue components **must not** be shared between different apps. Signed-off-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
Summary
solves an incompatibility issue when the providing app registers their code from an incompatible nextcloud-vue version.
Also changes and clarifies WorkflowEngine API. This is necessary to stay compatible with the original way, but also promotes usage of the originally declared but never used "component" attribute on registration.
TODO
Checklist