[RFC] Improve `spawnDialog` or add a new function

[ShGKme]

Problem

In the current implementation spawnDialog is designed to create a one-time "prompt" dialog:

• It doesn't provide control over the mounted dialog, for example, to unmount it or call its public methods
• Use case: https://github.com/nextcloud-libraries/nextcloud-dialogs/blob/main/lib/dialogs.ts#L80

But at the same time, it isn't convenient as a prompt, requiring to pass callback instead of returning a promise.

Proposals

1. Instead of only calling callback - return promise
   • How to determine if it was closed or unmounted?
2. Provide 2 functions:
   • One for one-time prompts returning a promise
   • One for mounting a dialog instance
3. Return promise with additional properties

Additional problem

In the current API props argument is passed as dialog component props, but it includes container and appContext, which are not dialog props.

We can mix it and only fix the implementation, or separate dialog's props and spawnDialog's options (breaking change).

[susnux]

Return promise with additional properties

You mean like extending a Promise? I do not like this 🙈

What are the requirements we have? So the use cases for the interface? What I see:

• allow to pass properties to dialog component
• allow to use global registered component in dialog component (not really see why you want to do this expect from styleguide)
• allow to use global (current app) registered Pinia (I see the use case e.g. for a settings store)
• allow to await dialog (close button / dialog buttons)
• allow to manually unmount (use case?)
• call exposed functions of the dialog (use case?)

So a simple interface would be:

function spawnDialog(component, props, dialogOptions)
// or
function spawnDialog(component, options: { props, options })

Maybe we can return an object?

interface SpawnedDialog {
    // to call exposed functions
    dialog: VueInstance
    // to get the result of the 'closed' event?
    // maybe throw a `DialogUnmounted` error if unmounted instead of closed?
    result: Promise<unknown>

[ShGKme]

What are the requirements we have? So the use cases for the interface? What I see:

I'd also think about reusable dialog. Like we do tight now with the password confirmation or unified search - it is rendered once and then only opens closes.

But it can also be a new function. Not only for dialogs.

[ShGKme]

• allow to use global registered component in dialog component (not really see why you want to do this expect from styleguide)
• allow to use global (current app) registered Pinia (I see the use case e.g. for a settings store)

This doesn't hurt (at least without Vapor), works already and nice-to-have feature. Also for debugger. So, I'd keep it.

• allow to manually unmount (use case?)

You were the last person used it =D

https://github.com/nextcloud-libraries/nextcloud-dialogs/blob/main/lib/dialogs.ts#L80

• allow to await dialog (close button / dialog buttons)

Maybe we can return an object?

This doesn't look nice for prompts

// With Promise
const wasConfirmed = await spawnDialog(Confirm)
const selectedConversation = await spawnDialog(ConversationSelector)

// With object
const wasConfirmed = await spawnDialog(Confirm).promise
const selectedConversation = await spawnDialog(ConversationSelector).promise

As promise is needed in 99% cases, maybe make it controlled?

const { vm, promise } = await spawnDialog(ConversationSelector, { controlled: true })

Or with different functions

const selectedConversation = await promptDialog(ConversationSelector)
const vm = spawnDialog(ConversationSelector)

[susnux]

In this case, for those two options, I would prefer having two functions. IMHO a function should only have one responsibility ("Functions should only do one thing" - Robert C. Martin) so overloading it with two different return types based on the options givens looks bad to me.

[ShGKme]

This doesn't hurt (at least without Vapor), works already and nice-to-have feature. Also for debugger. So, I'd keep it.

No, I was wrong, it hurts. With the current solution, the debugger doesn't work at all...

[ShGKme]

You mean like extending a Promise? I do not like this 🙈

Return PromiseLike instead of a Promise. Like we do with CancelablePromise.

It is a simple solution that covers both cases without making the API more complex.

IMHO a function should only have one responsibility ("Functions should only do one thing" - Robert C. Martin) so overloading it with two different return types based on the options givens looks bad to me.

It still does one job in my opinion - it mounts a one-time use prompt-like dialog.

Different return values are for basic and advanced usage.

And it's quite a common practice, for example, for composables with { controled: boolean } option. When it either return a ref, or an object with ref and function to control it

[ShGKme]

I thought a lot about this today, and I'd propose the following:

1. Do not toRaw result. If it's really required - add an option

Returning the result as toRaw is only needed for libraries to avoid exposing Vue, and can be For a general case, it is unnecessary, and might even be a problem.

Alternative: { raw: boolean } option if the library cannot be responsible for that.

2. Separate props and options and return Promise if not controlled

We shouldn't mix dialog's props with spawning options.

// Simple usage
declare function spawnDialog(dialog: Component, props: object): Promise

// With custom options
declare function spawnDialog(dialog: Component, props: object, options: object): Promise

// Old style = manual control
declare function spawnDialog(dialog: Component, props: object, onClose: Callback): { vm, unmount, promise }
declare function spawnDialog(dialog: Component, props: object, options: object, onClose: Callback): { vm, unmount, promise }

// Or when explicitly set
declare function spawnDialog(dialog: Component, props: object, { controled: true }): { vm, unmount, promise }

~~Alternative - PromiseLike~~

3. Use createApp instead of render(vnode)

• https://github.com/nextcloud-libraries/nextcloud-vue/pull/6752

• Otherwise there is no way to debug the dialog content in vue-devtools
  • IMHO, this is a critical issue
• We lose app context with registered components and injection context
  • But we can always provide necessary data explicitly, or via props

I'd say if we need to have the appContext - we should use a component/composable to actually have it in the rendering tree and setup context.

<script setup>
const dialog = useTemplateRef('dialogKey')

// when you need to prompt
// this opens dialog
// and resolves once it's closed
const result = await dialog.prompt()
</script>

<template>
  <!-- No v-if="showDialog", no @update:open, no callback configure -->  
  <NcDialog ref="dialogKey" />
  <!-- or -->
  <NcPromptDialog ref="dialogKey" :dialog :props />
</template>

[ShGKme]

• allow to manually unmount (use case?)

You were the last person used it =D

https://github.com/nextcloud-libraries/nextcloud-dialogs/blob/main/lib/dialogs.ts#L80

@susnux Could you confirm this use case is actual?

[susnux]

Return PromiseLike instead of a Promise. Like we do with CancelablePromise.

(and I wish we would have never done this and directly used standardized AbortController + vanilla promise 🙈 )

Extending promise is such a rabbit hole:

spawnDialog()
    .then(() => {})
    .catch(() => {})
    // this should work but in many implementations does not
    .callSomeExtendedFunction()

Personally I prefer simple return types like an object thats alway the same like above.

const { promise, vm } = spawnDialog(...)
vm.callSomeExtendedFunction(...)
await promise

// I do not consider this really overhead
const { promise } = spawnDialog()
await promise
// or
await spawnDialog().promise

But also this looks ok to me:

// plain
await spawnDialog(component)
// with VM
const { promise, vm } = spawnDialog(component, { controlled: true })

[susnux]

Could you confirm this use case is actual?

You mean that we should have a hide function? This is basically the only use case why I added the possibility to unmount in the orginal spawnDialog 😅

[susnux]

I'd say if we need to have the appContext - we should use a component/composable to actually have it in the rendering tree and setup context.

I agree that global components do not matter - just register locally. The use case with accessing the apps stores is pretty usual, in this case you need to explicitly pass the Pinia you want to use. But all this use cases can easily migrated by the app author.

[ShGKme]

You mean that we should have a hide function? This is basically the only use case why I added the possibility to unmount in the orginal spawnDialog 😅

Yes. I see we have the hide function, but do we need it? To hide a dialog from outside when it is open (so user cannot interact with anything outside).

I don't know what could trigger hide in theory...

It's also wierd for me, that we want to have unmount, but we don't control mount or remount. I can understan fully-controlled dialog, but not "unmount".

[ShGKme]

The use case with accessing the apps stores is pretty usual, in this case you need to explicitly pass the Pinia you want to use.

Pinia should work out of the box at least with one Pinia instance.

If there are many - you can provide it manually

[ShGKme]

Personally I prefer simple return types like an object thats alway the same like above.

Then it's always overloaded only to support a rare case

[susnux]

Pinia should work out of the box at least with one Pinia instance.

Nextcloud ecosystem -> there are many different apps.

I can understan fully-controlled dialog, but not "unmount".

Basically this comes from the JQuery dialogs migration where the api was like this. Probably because you had to manually mess with the DOM content of the dialog and handle the closing.

I would also be fine with deprecating this in general as this not really makes sense with the way our Vue dialogs work. Because as soon as you spawn the dialog the execution flow should move to the dialog and only return to the creating API when the dialog was closed / resolved.

[susnux]

Is something left @ShGKme ?

[ShGKme]

Is something left @ShGKme ?

Backport forward compatible version to v8 👀

[susnux]

ShGKme removed a link to a pull request

Something missing?

[ShGKme]

I'd like to backport Promise API in a non-breaking way for forward compatibility. I have a draft left from this option in the next version.