Skip to content

Comments

feat!: migrate to vue 3#967

Merged
susnux merged 1 commit intomainfrom
vue3
May 8, 2025
Merged

feat!: migrate to vue 3#967
susnux merged 1 commit intomainfrom
vue3

Conversation

@st3iny
Copy link
Contributor

@st3iny st3iny commented Apr 3, 2025

I tested this with twofactor_webauthn and it works fine.

Caveat

This is a breaking change and we need to maintain two versions for the foreseeable future. One for Vue 2 and one for Vue 3.

TODO

  • Clarify if we can drop the window global to store the dialog -> Yes, we can

@st3iny st3iny added enhancement New feature or request 2. developing Work in progress labels Apr 3, 2025
@st3iny st3iny self-assigned this Apr 3, 2025
src/main.ts Outdated
Comment on lines 86 to 89
function getDialog(validate: (password: string) => Promise<void>): { dialog: App, eventBus: Emitter<PasswordDialogEvents> } {
console.debug('Prompting password form')
const eventBus = mitt<PasswordDialogEvents>()
const dialog = spawnDialog(PasswordDialogVue, { eventBus, validate }, () => {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need an event bus here to handle closing.

If we removed _nc_password_confirmation_dialog, we can just pass onClose handler here directly as we are passing props.

If we want to reuse the same dialog, we can implement the prompt as the dialog's exposed public method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to Louis and he confirmed that we can drop the global dialog handle.

This simplifies the whole logic and would in theory open up the possibility to use the same code for both Vue versions (vue-demi?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simplifies the whole logic and would in theory open up the possibility to use the same code for both Vue versions (vue-demi?).

No. vue-demi works well for pure vue libraries, but here we have many dependencies, and they are not compatible with Vue in a "demi" way. Every dependency must also be based on vue-demi, or not use anything changed in Vue 3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support both versions in practice, we can bundle Vue in this library. Then it doesn't matter, what Vue version a library user uses.

However, it has a drawback - it makes the library MUCH bigger in the bundle size. I'll check if we can make it not so big. But it will be several times bigger than the current size.

Copy link
Contributor

@ShGKme ShGKme Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a spawnDialog function now in the nextcloud-vue.

https://github.com/nextcloud-libraries/nextcloud-vue/blob/main/src/functions/dialog/index.ts

Once it's backported to nextcloud-vue@8, we can use it in this library. Then the only difference between Vue 2 and Vue 3 versions is dependency versions.

This is also why we used forward compatible features:

@st3iny st3iny added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 6, 2025
@st3iny st3iny marked this pull request as ready for review May 6, 2025 11:06
@st3iny st3iny requested a review from susnux May 6, 2025 11:06
@st3iny st3iny requested a review from susnux May 7, 2025 11:34
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only docs need to be adjusted otherwise fine for migration

Signed-off-by: Richard Steinmetz <[email protected]>
@st3iny
Copy link
Contributor Author

st3iny commented May 8, 2025

I squashed without rebasing and applied Ferdinand's suggestion. Nothing else was changed.

@susnux susnux merged commit 9e41b48 into main May 8, 2025
11 checks passed
@susnux susnux deleted the vue3 branch May 8, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants