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

[full-ci] Vite #7952

Merged
merged 15 commits into from
Dec 8, 2022
Merged

[full-ci] Vite #7952

merged 15 commits into from
Dec 8, 2022

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Nov 13, 2022

Description

This makes oC Web build with Vite.

To test this you need to do docker-compose up -d.
OAuth2 Application in oC 10 and OIDC Client in oCIS are setup automatically now.

Motivation and Context

rollup builds are rather slow and we spend a lot of time waiting for builds during development.
vite supports Hot Module Reloading which makes changes instant. If there are errors they are shown in the browser and we don't need to switch to the terminal to notice them.
Moreover it's the standard tool in the Vue world by now.
It uses rollup as a bundling backend in production mode and has compatible plugin api, so we need to worry less about standard rollup plugins (because they are abstracted away by vite) and can still use more exotic rollup plugins.

Screenshots (if appropriate):

Open tasks:

Followup tasks:

  • Reenable cssCodeSplit with Vue 3 (or when vue2 plugin supports it)
  • Use vitest
  • Make browserlist available as esbuild target, possibly with: Support browserlist as target option evanw/esbuild#324
  • Port external apps to vite
    • ocis settings
    • web-app-gpx-viewer 🤓
    • web-app-skeleton
    • ....
    • ensure vue, vuex and luxon are not embedded in external apps
  • Clean up config.json files (?)
  • Fix production rebuilds require docker-compose restart oc10. The dist folder is completely emptied and so our bind mounts break, dist cannot be mounted completely because of the file structure and where the php files need to be put
    -> do not merge dist folder with app.js and php files but have it in a separate sub folder
  • Add gzipped assets to production builds
  • enable history mode for pnpm vite / pnpm vite:ocis (not for oc10!)
  • check bundle size: compare esbuild vs terser
  • do not ship config.json in prod builds
  • downloads in ocis dev server often fail, possibly because of the proxy -> get rid of proxy
  • ensure typescript types are actually checked at some point

@update-docs
Copy link

update-docs bot commented Nov 13, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@dschmidt dschmidt changed the title WIP: Make it possible to develop with Vite WIP: Vite Nov 13, 2022
@dschmidt dschmidt force-pushed the rollup-3 branch 6 times, most recently from b42df9f to fbffd7a Compare November 15, 2022 10:55
@delete-merged-branch delete-merged-branch bot deleted the branch master November 15, 2022 11:28
@dschmidt dschmidt changed the base branch from rollup-3 to master November 18, 2022 00:28
@dschmidt dschmidt changed the base branch from master to owncloud-design-system-workspace November 18, 2022 00:28
@dschmidt dschmidt force-pushed the owncloud-design-system-workspace branch 3 times, most recently from 6079d5c to 5530b9b Compare November 21, 2022 15:32
@delete-merged-branch delete-merged-branch bot deleted the branch master November 21, 2022 16:20
@dschmidt dschmidt changed the base branch from owncloud-design-system-workspace to master November 21, 2022 18:25
@fschade
Copy link
Contributor

fschade commented Nov 21, 2022

Just read it on my cell… ❤️ it… I give it a try tomorrow! Thank you 😘

@dschmidt dschmidt force-pushed the vite branch 3 times, most recently from 4f94082 to f0f7130 Compare November 25, 2022 20:38
packages/web-app-files/src/services/folder.ts Show resolved Hide resolved
packages/web-runtime/src/layouts/Loading.vue Outdated Show resolved Hide resolved
config/vite_oc10/config.json Outdated Show resolved Hide resolved
@dschmidt dschmidt force-pushed the vite branch 5 times, most recently from d212e7d to 3163034 Compare December 5, 2022 21:04
@dschmidt dschmidt changed the title WIP: Vite [full-ci] Vite Dec 5, 2022
dschmidt and others added 11 commits December 7, 2022 17:45
Allows better error reporting and prepares us for using importmaps later.
In a few places we rely on whitespaces for styling 👀 (e.g., AppTopBar buttons and in the account menu)
... in order to not break those occasions, we set this setting for now.
"search",
"external",
"user-management",
"preview"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mount preview as external app, same as in config.json.sample-ocis. The thumbnailer in ocis supports more mimetypes than the preview-app defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry... not in the file I mentioned. Anyway, please add the preview as follows to the external apps and remove it from the apps array:

{
      "id": "preview",
      "path": "web-app-preview",
      "config": {
        "mimeTypes": ["image/tiff","image/bmp","image/x-ms-bmp"]
      }
    }

Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

I think this still has some rough edges for devs:

  • router is in hash mode (that's controlled server side at the moment, directly in ocis via modification of the index.html)
  • public links are announced by the server and as such are not with port 9201 when running in pnpm vite-dev-mode.

Both of those are not blockers for merging the PR. But it's really good that running the dev setup with vite is not documented by this PR, yet. Since the production build and build:w still work the same in the dev setup, and that is what we've documented, I see no blockers for this PR.

Love how you modernized a whole lot of things. And HMR of course. 😁 ❤️ Let's get this merged asap.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 3 Security Hotspots
Code Smell A 5 Code Smells

4.1% 4.1% Coverage
0.0% 0.0% Duplication

@dschmidt
Copy link
Member Author

dschmidt commented Dec 8, 2022

I think this still has some rough edges for devs:

  • router is in hash mode (that's controlled server side at the moment, directly in ocis via modification of the index.html)

Good catch. Indeed, we need to add a base tag to enable it. Can be achieved with vite like this:

       {
          name: 'add-base',
          transformIndexHtml: {
            transform() {
              return [
                {
                  tag: 'base',
                  attrs: {
                    'href': '/'
                  }
                }
              ]
            }
          }
        }

I would just add it, but it's causing an endless redirect loop for me and I have no time to debug right now.

  • public links are announced by the server and as such are not with port 9201 when running in pnpm vite-dev-mode.

Yeah, not sure, what to do about that ... I'm afraid our only way of dealing with this, is rewriting the urls client side. Everything else would have quite some complexity server side and I doubt the server team(s) would be happy.

Both of those are not blockers for merging the PR. But it's really good that running the dev setup with vite is not documented by this PR, yet. Since the production build and build:w still work the same in the dev setup, and that is what we've documented, I see no blockers for this PR.

Love how you modernized a whole lot of things. And HMR of course. grin heart Let's get this merged asap.

💪🏻

@kulmann kulmann merged commit 7e10c02 into master Dec 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the vite branch December 8, 2022 15:11
ownclouders pushed a commit that referenced this pull request Dec 8, 2022
Author: Dominik Schmidt <dschmidt@owncloud.com>
Date:   Thu Dec 8 16:11:27 2022 +0100

    [full-ci] Vite (#7952)
@kulmann
Copy link
Contributor

kulmann commented Dec 8, 2022

@diocas @elizavetaRa do you still rebase to master? If yes then you might experience some issues with your other apps - please reach out, we'll help you fix issues. Also, please note that data.config.cdn is now data.buildConfig.cnd in the index.html file. Again, if you need help fixing stuff, please reach out.

@pascalwengerter
Copy link
Contributor

Lovely! oC web, welcome to the wild 20s of the 21 century :D I'm {proud of,happy for} y'all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants