bug(NcButton): does not work as `router-link` anymore.

[raimund-schluessler]

This got completely lost in the refactoring. NcButton does not work as a router-link anymore.

Originally posted by @raimund-schluessler in 2c92c40

NcButton is not wrapped in a router-link element anymore when the to prop is set. This breaks quite some places, most prominently the use in NcBreadcrumb. While the app still navigates when to is set, the element does not behave as a router-link should, as it is not an a element anymore.

[raimund-schluessler]

@susnux Unfortunately, I am not well familiar with script setup yet and I am not sure I can fix this well. If you would have time to look into this, that would be great!

[raimund-schluessler]

Regression from #6033.

[ShGKme]

Did you test with npm link or installed?

[raimund-schluessler]

Did you test with npm link or installed?

I did test both, latest main via npm link and installed 9.0.0-rc.2, but it does not make a difference. Both are affected.

[ShGKme]

@raimund-schluessler Does it happen on production, or on development to you?

[raimund-schluessler]

I only tested development, I think.

[raimund-schluessler]

Although, no, now that I think about I tested the last RC and that must've been a production build. But development I tested after, and it also happens there.

[ShGKme]

I only tested development, I think.

For us it only happend on mixed build: production app build with dev nextcloud/vue or dev build with production lib.

[raimund-schluessler]

I cannot test it right now, sorry.

[raimund-schluessler]

I tried again now with RC4 and a production build, and it still happens for me when used in NcBreadcrumb. I didn't test a pure NcButton though. But looking at the code of NcButton, it seems to me it is never rendered as an a if only :to is provided:

https://github.com/nextcloud-libraries/nextcloud-vue/blob/10f74796ed9d7bc916e4d058b8745030fc33dd8d/src/components/NcButton/NcButton.vue#L687

since isLink is only true if one provides an :href. This is a different behaviour than a standard router-link as it was before #6033.

[raimund-schluessler]

This still does not work for me. In NcButton const hasVueRouterContext = inject(routerKey, null) !== null is always false, although the app uses VueRouter. Is there any adjustment required for this to work?

[susnux]

There are unit tests covering this 👀 How do you register the router?

[raimund-schluessler]

It is registered here: https://github.com/raimund-schluessler/inventory/blob/17e98514b6783722e755893223f5d9b0e8104077/src/main.js#L25-L40

and the router is created here: https://github.com/raimund-schluessler/inventory/blob/17e98514b6783722e755893223f5d9b0e8104077/src/router.js#L250-L253

[raimund-schluessler]

There are unit tests covering this 👀 How do you register the router?

Maybe this only works in tests. As the docs say this is Internal and "Allows overriding the router instance returned by useRouter in tests.": https://router.vuejs.org/api/variables/routerKey.html

[susnux]

Maybe this only works in tests.

No it is basically the full implementation of useRouter:

function useRouter() {
    return inject(routerKey);
}

[ShGKme]

We could use useRouter as a public API, but it has no default value and warns if there is no vue-router

[susnux]

I checked your app and I cannot reproduce this.

Steps I did:

1. checkout current main branch of nextcloud-vue
2. run npm build
3. run npm pack
4. checkout current main branch of inventory
5. update dependency to the packed one of nextcloud-vue
6. run npm dev
7. Run app in Nextcloud

[ShGKme]

@raimund-schluessler in your package.json you have all dependencies in fixed versions. It is not correct in most of the cases. It means, for example, if some lib needs ^1.2.3 version and your app says 1.2.2, then this lib will be installed twice in slightly different versions, even a single version works fine.

I don't see it a problem for this specific issue, but it will cause issues with dependencies.

[susnux]

We could use useRouter as a public API, but it has no default value and warns if there is no vue-router

Well I think it is somewhat public if their developers suggesting me to use that. And also they state it in their docs to work for tests - not sure how likely it is to break as that would break all tests 🤷

But then I would rather use this solution:

const hasVueRouter = typeof resolveComponent('RouterLink')  !== 'string

[raimund-schluessler]

I checked your app and I cannot reproduce this.

Steps I did:

1. checkout current main branch of `nextcloud-vue`

2. run `npm build`

3. run `npm pack`

4. checkout current main branch of `inventory`

5. update dependency to the packed one of nextcloud-vue

6. run `npm dev`

7. Run app in Nextcloud

Thanks for checking, this is weird. I do this and it does not work, neither NcButton nor NcBreadcrumbs are router-links:

1. Checkout current main of both nextcloud-vue and the app and install deps with npm ci.
2. Build nextcloud-vue with npm run build.
3. Link it with npm link.
4. Link it in the app with npm link @nextcloud/vue.
5. Build the app with npm run build.

[raimund-schluessler]

@raimund-schluessler in your package.json you have all dependencies in fixed versions. It is not correct in most of the cases. It means, for example, if some lib needs ^1.2.3 version and your app says 1.2.2, then this lib will be installed twice in slightly different versions, even a single version works fine.

Thanks for the hint. That is a leftover of the time the app was using renovate bot to keep the deps up-to-date. It insisted on this. I will change it.

[ShGKme]

@raimund-schluessler What nodejs and npm version you check with? Could you provide detailed steps to reproduce or illustrate the issue? Screenshots of the problem, for example. Or an archive with the built files to compare them with our or to check in browsers.

[susnux]

Also @raimund-schluessler please check explicitly with npm pack solution instead of npm link - for me linking rather never works as intended.

[ShGKme]

npm link should never be used in this scenario. With npm link dependency resolving is not used. Vue, vue-router and other dependencies that are stateful and supposed to be shared are duplicated.

[ShGKme]

nextcloud-vue
| node_modules
| | vue 🔷
| | vue-router 🔷
| dist
| | main.js
| |   import { ... } from 'vue' (resolves as ../node_modules/vue🔷)
| |   import { ... } from 'vue-router' (resolves as ../node_modules/vue-router🔷)
app
| node_modules
| | vue ♦️
| | vue-router ♦️
| js
| | main.js
| |   import { ... } from 'vue' (resolves as ../node_modules/vue♦️)
| |   import { ... } from 'vue-router' (resolves as ../node_modules/vue-router♦️)
| |   import { ... } from '@nextcloud/vue' (uses vue🔷 and vue-router🔷)

[ShGKme]

In Vue it breaks rendering and reactivity. In Vue-Router it makes app and lib using different injection symbol

[raimund-schluessler]

That could be the explanation why it doesn't work for me then. I will try with npm pack in the evening and report.

[raimund-schluessler]

Thanks a lot for your help. Indeed, the problem was npm link. With npm pack it works fine.