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

Migrate to vue 3 + vite / vitest / vitepress for building #3565

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Dec 12, 2022

Based on #2637 and #3529

The migration to vite does only make sense using vue 3 (so the automatic docs generation works).


  • Migrate to vite for building (so dependency on webpack anymore)
  • Migrate to vitest for unit tests (instead of jest)
  • Migrate to vue-docgen + vitepress for documentation
  • Migrate cypress to use vite (and updated cypress)
  • 🚧 vue 3 migration (under development by @raimund-schluessler )

@susnux susnux added the 2. developing Work in progress label Dec 12, 2022
@susnux susnux added dependencies Pull requests that update a dependency file breaking PR that requires a new major version labels Dec 12, 2022
@susnux susnux force-pushed the feat/vue3+vite branch 2 times, most recently from d11c189 to 4bfcd72 Compare December 12, 2022 20:38
@susnux
Copy link
Contributor Author

susnux commented Dec 12, 2022

Currently the netlify deploys are broken, and there is no log, so here is how it looks locally:

voko.mp4

NcContent does now work (did not even work on the current master branch).

on master 🖼️ with this changes 🖼️ ✨
Screenshot_20221212_225845 Screenshot_20221212_225906

@raimund-schluessler

This comment was marked as outdated.

@raimund-schluessler

This comment was marked as resolved.

@raimund-schluessler
Copy link
Contributor

How do we best proceed here? Do we simply push our changes to the components to this branch? Should we work with PRs?

@raimund-schluessler

This comment was marked as resolved.

@raimund-schluessler

This comment was marked as resolved.

@susnux susnux force-pushed the feat/vue3+vite branch 6 times, most recently from d9305c9 to 6533f51 Compare December 14, 2022 18:06
@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

How do we best proceed here? Do we simply push our changes to the components to this branch? Should we work with PRs?

Are you talking about vue 3 changes? I do not know, do you want to build on top of this or keep the changes separated in your vue 3 branch?

If the latter one, then if you push new commits to it (instead of squashing / amending the commit) we could rebase this on top of the vue 3 branch.

If you would like to build on top of this, then probably simply pushing is easier.
Do you need / want help with the migration? I would be happy to help :)

@raimund-schluessler
Copy link
Contributor

Do you need / want help with the migration? I would be happy to help :)

Help is always welcome 🙂 There is still a lot to do for vue3, especially all components that use this.$slots currently don't work correctly and likely need to be changed to use a render function instead of a template.

I am a bit undecided whether to do the vue3 work here or in the vue3 branch. The changes are massive and merging vite with vue3 will increase the amount of changes further. But doing both together also makes a lot of sense, since then we can update all dependencies at once and have everything working at some point. So I tend to to the migration here. Maybe @skjnldsv has some input as well regarding how to proceed best. Vue3 needs to become the default this year anyway, since support for vue2 will be dropped.

@susnux I don't have experience with vite/vitepress, so it would be great if you could have a look again at these errrors:

[postcss] Failed to find 'server.css'
  in [
    /home/runner/work/nextcloud-vue/nextcloud-vue/docs/.vitepress/theme/assets
  ]
Error: R] No matching export in "node_modules/vue/dist/vue.runtime.esm-bundler.js" for import "default"

    node_modules/cypress/vue2/dist/cypress-vue2.esm-bundler.js:8:7:
      8 │ import Vue from 'vue';
        ╵        ~~~

11:07:33 PM [vite] error while updating dependencies:
TypeError: Cannot read properties of undefined (reading 'outputs')
    at runOptimizeDeps (file:///home/runner/work/nextcloud-vue/nextcloud-vue/node_modules/vite/dist/node/chunks/dep-6305614c.js:43946:49)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async optimizeNewDeps (file:///home/runner/work/nextcloud-vue/nextcloud-vue/node_modules/vite/dist/node/chunks/dep-6305614c.js:43267:16)
    at async runOptimizer (file:///home/runner/work/nextcloud-vue/nextcloud-vue/node_modules/vite/dist/node/chunks/dep-6305614c.js:43296:55)

@skjnldsv
Copy link
Contributor

skjnldsv commented Jan 6, 2023

Fully rebased! We now have some typescript files @susnux
Feel free to take advantage of those and migrate some mjs configs to it.

"vue-select": "^4.0.0-beta.6"
},
"peerDependencies": {
"vue": "^3.2.45"
Copy link
Contributor

Choose a reason for hiding this comment

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

vue should also be in the dependencies section :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Jan 6, 2023

Maybe @skjnldsv has some input as well regarding how to proceed best. Vue3 needs to become the default this year anyway, since support for vue2 will be dropped.

Well, apps with vue 3 can use @nextcloud/vue on vue 2
BUT not the other way around. So it's nice that we have this being done, but it will be unusable as long as we don't have a clear migration path for all apps to go from 2 to 3

Especially with https://github.com/nextcloud/webpack-vue-config

@raimund-schluessler
Copy link
Contributor

Especially with https://github.com/nextcloud/webpack-vue-config

@skjnldsv There is a draft for the required adjustments to the webpack config in nextcloud-libraries/webpack-vue-config#331 and a WIP for moving the Tasks app to vue3 nextcloud/tasks#1971

@skjnldsv
Copy link
Contributor

skjnldsv commented Jan 6, 2023

@skjnldsv There is a draft for the required adjustments to the webpack config in nextcloud/webpack-vue-config#331 and a WIP for moving the Tasks app to vue3 nextcloud/tasks#1971

Maybe we should try a tiny app, like guests first 🙈

raimund-schluessler and others added 11 commits January 29, 2023 12:06
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Jan 30, 2023

@susnux Here is what I noticed with the feat/vue3+vite branch so far:

  • I don't know if this is a deliberate choice, but importing via import NcActions from '@nextcloud/vue/dist/Components/NcActions.js' does not work anymore. import { NcActions } from '@nextcloud/vue' of course works.
  • The style sections of the vue components don't seem to be bundled anymore. Do we have to explicitly import them now, or is this a bug?
  • The component preview in the docs does not work yet. It only show loading....
  • When running npm run styleguide locally, I get this output
[1] failed to load config from /home/raimund/nextcloud/apps2/nextcloud-vue/docs/.vitepress/config.mjs
[1] failed to start server. error:
[1]  Error: ENOENT: no such file or directory, scandir '/home/raimund/nextcloud/apps2/nextcloud-vue/docs/components'
[1]     at Module.readdirSync (node:fs:1390:3)
[1]     at getComponents (file:///home/raimund/nextcloud/apps2/nextcloud-vue/docs/.vitepress/config.mjs.timestamp-1675113174975.mjs:175:6)
[1]     at file:///home/raimund/nextcloud/apps2/nextcloud-vue/docs/.vitepress/config.mjs.timestamp-1675113174975.mjs:223:16
[1]     at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
[1]     at async Promise.all (index 0)
[1]     at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
[1]     at async loadConfigFromBundledFile (file:///home/raimund/nextcloud/apps2/nextcloud-vue/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:62084:21)
[1]     at async loadConfigFromFile (file:///home/raimund/nextcloud/apps2/nextcloud-vue/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:61969:28)
[1]     at async resolveUserConfig (file:///home/raimund/nextcloud/apps2/nextcloud-vue/node_modules/vitepress/dist/node/serve-a99591d0.js:10926:27)
[1]     at async resolveConfig (file:///home/raimund/nextcloud/apps2/nextcloud-vue/node_modules/vitepress/dist/node/serve-a99591d0.js:10858:48) {
[1]   errno: -2,
[1]   syscall: 'scandir',
[1]   code: 'ENOENT',
[1]   path: '/home/raimund/nextcloud/apps2/nextcloud-vue/docs/components'
[1] }
[1] vitepress dev docs exited with code 1

Since the migration to vite and vitepress creates further complexity, I will work with the vue3 branch in #3692 for the moment in order to focus on the vue3 part until I manage to properly compile apps with the vite branch.

@raimund-schluessler
Copy link
Contributor

Btw, lint is also not happy here about the nextcloud/l10n package:

/home/runner/work/nextcloud-vue/nextcloud-vue/src/l10n.js
Error:   1:35  error  Unable to resolve path to module '@nextcloud/l10n/gettext'  import/no-unresolved

@susnux
Copy link
Contributor Author

susnux commented Jan 31, 2023

@raimund-schluessler

* I don't know if this is a deliberate choice, but importing via `import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'` does not work anymore. `import { NcActions } from '@nextcloud/vue'` of course works.

This is done on purpose, bring back an import for each component is possible if this is a desired feature.

* The `style` sections of the vue components don't seem to be bundled anymore. Do we have to explicitly import them now, or is this a bug?

This is no bug, but can be disabled (keep inlining the CSS), see the vite config flag

* The component preview in the docs does not work yet. It only show `loading...`.

Do you have this problem with the latest commits? Then I have to investigate what is failing here.

* When running `npm run styleguide` locally, I get this output

I got the same, but it works if you run the command twice. I will try to fix this.

Since the migration to vite and vitepress creates further complexity, I will work with the vue3 branch in #3692 for the moment in order to focus on the vue3 part until I manage to properly compile apps with the vite branch.

I fully understand. I will try to fix the vite issues (or to be more specific the styleguide issues...) and push fixes for vue3 to the vue3 branch instead.
I will then try to keep this branch based ontop of the vue3 branch.

@raimund-schluessler
Copy link
Contributor

* I don't know if this is a deliberate choice, but importing via `import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'` does not work anymore. `import { NcActions } from '@nextcloud/vue'` of course works.

This is done on purpose, bring back an import for each component is possible if this is a desired feature.

I don't have a strong opinion here. I guess the import via dist is not really necessary anymore with vite to reduce the bundle size. But not having it creates further migration effort. From my side, both would be fine.

* The `style` sections of the vue components don't seem to be bundled anymore. Do we have to explicitly import them now, or is this a bug?

This is no bug, but can be disabled (keep inlining the CSS), see the vite config flag

Is there any drawback from inlining it? To me, having it inline feels more natural.

* The component preview in the docs does not work yet. It only show `loading...`.

Do you have this problem with the latest commits? Then I have to investigate what is failing here.

Yes, latest state of the PR. Just look at https://deploy-preview-3565--nextcloud-vue-components.netlify.app/components/NcActions.html for example. The component preview just shows loading ....

* When running `npm run styleguide` locally, I get this output

I got the same, but it works if you run the command twice. I will try to fix this.

Interesting. I didn't try running it twice.

Since the migration to vite and vitepress creates further complexity, I will work with the vue3 branch in #3692 for the moment in order to focus on the vue3 part until I manage to properly compile apps with the vite branch.

I fully understand. I will try to fix the vite issues (or to be more specific the styleguide issues...) and push fixes for vue3 to the vue3 branch instead. I will then try to keep this branch based ontop of the vue3 branch.

👍 Sounds good to me.

@susnux
Copy link
Contributor Author

susnux commented Mar 7, 2023

I am currently rebasing this, and it looks good, the most issues are resolved (e.g. the documentation issue).
But this is currently blocked by broken ESM implementation of @nextcloud/event-bus and floating-vue:

@raimund-schluessler
Copy link
Contributor

@susnux Since the components got migrated to vue 3 completely, and we use vite and vitest already, do you still plan to migrate to vitepress now?

@susnux
Copy link
Contributor Author

susnux commented Dec 21, 2023

Since the components got migrated to vue 3 completely, and we use vite and vitest already, do you still plan to migrate to vitepress now?

I think it still makes sense, no? In best case we can drop one bundler (webpack) and just use vite :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress breaking PR that requires a new major version dependencies Pull requests that update a dependency file vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants