Skip to content
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

fix: Do not depend on vue or vue-router for the public interface of OC.Files.Router #255

Merged
merged 1 commit into from
May 29, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 27, 2024

Having vue types is not a good idea, this will become a problem if you depend on multiple versions of vue-router.

So for this functions are basically only provided for triggering route changes, so we can simply set them to Promise return type.

So we should bundle the exact types used on that exact server version.

@ShGKme
Copy link

ShGKme commented May 27, 2024

Do you have an example of such problems?

@ShGKme
Copy link

ShGKme commented May 27, 2024

When I checked the last time, some app used the route return by promise

…OC.Files.Router`

Instead bundle interfaces used on the specific server version.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented May 27, 2024

I am thinking of situations like having vue-router v4 but server is still using vue-router v3.

Or later if we use vue router v4 and they change the interface we need to depend on multiple versions of vue-router for every server version.

So we can instead bundle the types used.


Myself I only have the problem of having two versions of vue and vue-router installed for vue 3 apps, leading to EOL warnings + unnecessary large node_modules.

@ShGKme
Copy link

ShGKme commented May 27, 2024

I am thinking of situations like having vue-router v4 but server is still using vue-router v3.

But is it a problem? npm allows different dependencies to have different sub-dependencies. We also have sometimes @nextcloud/vue 7 and 8 in parallel or @nextcloud/router 2 and 3.

It is usually only a problem if you directly have conflicting dependencies and peer dependency of a dependency which is not the case for @nextcloud/typings

Or later if we use vue router v4 and they change the interface, we need to depend on multiple versions of vue-router for every server version.

Yes, but it is also solvable in npm if needed. It allows having parallel dependencies in different versions. Or we can stop exposing directly an interface from an external library in future versions 👀

unnecessary large node_modules

Yes, but for server it's less than 1% diff for node_modules and even less for a whole repo


So, IMO, there are no serious issues of having the correct type here.

And if we need to get rid of it, we can at least provide a limited type for apps that currently use the route object from the result. It is not void for them.

@susnux
Copy link
Contributor Author

susnux commented May 28, 2024

And if we need to get rid of it, we can at least provide a limited type for apps that currently use the route object from the result. It is not void for them.

I already changed it to the extracted interface instead of void

@susnux susnux merged commit 8f0839f into master May 29, 2024
5 checks passed
@susnux susnux deleted the fix/do-not-depend-on-vue branch May 29, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants