Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Merge components into @nextcloud/vue #407

Closed
raimund-schluessler opened this issue May 1, 2022 · 10 comments
Closed

Merge components into @nextcloud/vue #407

raimund-schluessler opened this issue May 1, 2022 · 10 comments
Labels
discussion Being discussed question Further information is requested

Comments

@raimund-schluessler
Copy link
Contributor

I would like to propose to merge the two Dashboard components into nextcloud/vue (sorry if this has been discussed already).

The short reason for this is that the effort of maintaining a separate repository for only two components is not worth the rather ideological benefit of doing so.

The longer list of reasons for merging is:

  • The nextcloud-vue-dashboard repository is quite outdated (showing that the effort is quite high):
    • It still uses nextcloud/vue@3. This would be immediately tackled if the Dashboard components would live in nextcloud/vue itself.
    • The whole rollup config is old and rollup-plugin-vue is not even maintained anymore (they recommend using vite and @vitejs/plugin-vue now).
  • To my knowledge, this repo is the only one using rollup yet. While rollup seems nice, this more than doubles the effort to maintain nextcloud/vue and nextcloud/vue-dashboard separately.
  • The repository has basically no dependencies besides nextcloud/vue. Merging it into nextcloud/vue would not increase the package size of either package.
  • All of the above makes the migration towards vue 3 more cumbersome as if it would live in nextcloud/vue. While the components themself don't need any change for vue 3 (just a small one for nextcloud/vue@5), the configuration is a bit challenging (for me, I couldn't get it to work properly with vue 3 yet, see my try in Migrate to vue 3 #406).

The only rational against merging I could think of is that the two components are not used within an app, hence, don't strictly belong to components logically. But in my opinion this is more than outweighed by the above reasons.

@eneiluj @juliushaertl @nickvergessen @marcoambrosini @skjnldsv Since you contributed the most here.

In case you are ok with this approach, I am happy to send a PR with the necessary adjustments to nextcloud/vue. (In fact, I have the two components in my nextcloud/vue#vue3 nextcloud-libraries/nextcloud-vue#2637 branch already, since I couldn't get it to work as a standalone repo yet, npm link is a challenge here as well).

@raimund-schluessler raimund-schluessler added discussion Being discussed question Further information is requested labels May 1, 2022
@nickvergessen
Copy link
Contributor

I think this is more due to historic reasons and in the beginning we thought it might be better to have a set of small things that do exactly 1 thing and not 1 giant lib to do everything, but I'm only a consumer of the repo. I have 0 clue about packaging/rollup and other strategies

@juliusknorr
Copy link
Collaborator

Yes, I'd agree that it probably makes more sense to move those components into the library these days.

The only rational against merging I could think of is that the two components are not used within an app, hence, don't strictly belong to components logically. But in my opinion this is more than outweighed by the above reasons.

Would still be fine I'd say as @nextcloud/vue also allows individual imports of components so the app bundling can take care of only embedding the required parts.

@nickvergessen
Copy link
Contributor

@raimund-schluessler
Copy link
Contributor Author

Same for https://github.com/nextcloud/nextcloud-vue-collections I guess?

There I am not sure. nextcloud-vue-collections requires vuex and brings an own store, which nextcloud/vue does not. This could create issues. Nonetheless it's just as outdated, with the last relase 1.5 years ago 😄🙈

@raimund-schluessler
Copy link
Contributor Author

The PR to merge the Dashboard components is in nextcloud-libraries/nextcloud-vue#2668.

@raimund-schluessler
Copy link
Contributor Author

Done.

@nickvergessen
Copy link
Contributor

So we archive this after adding a last message to the readme?

@raimund-schluessler
Copy link
Contributor Author

So we archive this after adding a last message to the readme?

I would say so, see #419

@korelstar
Copy link

But I think there is no release of nc-vue with this change yet.

@nickvergessen
Copy link
Contributor

Doesn't matter? We will not remove existing packages from npm, so you can continue using those.
Archiving just means no one can create new issues/prs and also dependabot doesn't try to update things

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Being discussed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants