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

Build performance with vite #6732

Closed
mejo- opened this issue Dec 4, 2024 · 7 comments · Fixed by #6765
Closed

Build performance with vite #6732

mejo- opened this issue Dec 4, 2024 · 7 comments · Fixed by #6765
Assignees
Labels
bug Something isn't working javascript

Comments

@mejo-
Copy link
Member

mejo- commented Dec 4, 2024

Describe the bug
Since the migration to vite, building the javascript assets takes much longer. This is particularly annoying during development, when using npm run dev or npm run watch.

This is mostly due to our dependencies mermaid and highlight.js being really heavy and being rebuilt each time by vite.

Some ideas we had so far:

Build mermaid and highlight.js separately

  • Create a separate JS entrypoint that is built using a dedicated vite config.
  • In the main vite.config.ts mark these modules as external via build.rollupOptions.external.
  • The entrypoint registers a function in global namespace (e.g. `window.OCA.Text.modules) to initialize the modules, which loads the assets asynchronously and then returns them.

Problem: Building with vite clears the target directory upfront. So we cannot build assets into the same target directory using two separate vite processes.

Use vite serve to provide the javascript

vite serve is really fast.

  • Run NODE_ENV=development npm exec vite -- --mode development serve --host in Text starts the vite webserver that provides content on port 5173
  • The reverse proxy used to serve Nextcloud would need a rule to redirect requests for /apps/text/js/* to the vite webserver. On nginx from nextcloud-docker-dev this can be done the following way:
    upstream vite {
        server 192.168.15.1:5173;
    }
    server {
        ...
      rewrite ^/index.php/apps/text/js/text-text.mjs /index.php/apps/text/src/main.js;
      rewrite ^/index.php/apps/text/js/text-editors.mjs /index.php/apps/text/src/editor.js;
      rewrite ^/index.php/apps/text/js/text.(.*).mjs /index.php/apps/text/src/$1.js;
        location /index.php/apps/text/src/ {
            proxy_pass http://vite/js/;
            set $upstream_keepalive false;
        }
        ...
    }
    

Problem: I didn't manage to make vite provide all required files yet.

@juliusknorr @max-nextcloud feel free to adjust and add other ideas.

@mejo- mejo- added bug Something isn't working javascript labels Dec 4, 2024
@mejo- mejo- self-assigned this Dec 4, 2024
@github-project-automation github-project-automation bot moved this to 🧭 Planning evaluation (don't pick) in 📝 Office team Dec 4, 2024
@mejo- mejo- moved this from 🧭 Planning evaluation (don't pick) to 🏗️ In progress in 📝 Office team Dec 4, 2024
@mejo-
Copy link
Member Author

mejo- commented Dec 4, 2024

I made the second approach work using the following nginx config snippet, which certainly can be optimized:

    rewrite ^(/index.php)?/apps/text/js/text-text.mjs $1/apps/text/src/main.js;
    rewrite ^(/index.php)?/apps/text/js/text-editors.mjs $1/apps/text/src/editor.js;
    rewrite ^(/index.php)?/apps/text/js/text.(.*).mjs $1/apps/text/src/$2.js;
    location /index.php/apps/text/src {
        proxy_pass http://vite/src;
        proxy_redirect /src /apps/text/src;
    }
    location /apps/text/src {
        proxy_pass http://vite/src;
        proxy_redirect /src /apps/text/src;
    }
    location /node_modules {
        proxy_pass http://vite/node_modules;
    }
    location /src {
        proxy_pass http://vite/src;
    }
    location /@id {
        proxy_pass http://vite/@id;
    }
    location /@vite {
        proxy_pass http://vite/@vite;
    }

@mejo-
Copy link
Member Author

mejo- commented Dec 4, 2024

I managed to simplify the setup a lot:

  1. In vite.config.js, set config.base: process.env.BASE'
  2. Start with BASE=/apps/text NODE_ENV=development npm exec vite -- --mode development serve --host
  3. In nginx.conf.d/default.conf use the following:
        rewrite ^/apps/text/js/text-text.mjs /apps/text/src/main.js;
        rewrite ^/apps/text/js/text-editors.mjs /apps/text/src/editor.js; 
        rewrite ^/apps/text/js/text.(.*).mjs /apps/text/src/$1.js;
        location /apps/text {
            proxy_pass http://192.168.15.1:5173/apps/text;
            # fallback to nextcloud server if vite serve doesn't answer
            error_page 502 = @fallback;
        }
        location @fallback {
            proxy_pass http://nextcloud.local;
        }
    

This way even HRM updates via websockets work, so no page reloads needed anymore.

So far I tested it with Text in viewer and Collectives.

If we find a way to make the reverse proxy configuration in nginx switchable that could be a good way forward in my eyes.

@max-nextcloud
Copy link
Collaborator

I wonder if something like this could work: https://stackoverflow.com/a/64060513

    location /apps/text {
        proxy_pass http://192.168.15.1:5173/apps/text;
        error_page 502 = @fallback;
    }

    location @fallback {
        proxy_pass http://nextcloud.local;
    }

So if vite is not running fall back to the default nextcloud server.

@juliusknorr
Copy link
Member

The reverse proxy used to serve Nextcloud would need a rule to redirect requests for /apps/text/js/* to the vite webserver. On nginx from nextcloud-docker-dev this can be done the following way

I can imagine we just mount a user editable config file to the container so people could add the config for their apps more easily. Don't think there is a nice way to auto configure that as we'll always need the manual port to app path mapping.

mejo- added a commit that referenced this issue Dec 10, 2024
@mejo-
Copy link
Member Author

mejo- commented Dec 10, 2024

I found a way to do the redirects in vite and opened a PR that adds support for vite serve with only the following snippet left to be added to the nginx config (needs to go into the server section for nextcloud.local):

    location /apps/text {
        proxy_pass http://192.168.15.1:5173/apps/text;
        # fallback to nextcloud server if vite serve doesn't answer
        error_page 502 = @fallback;
    }
    location @fallback {
        proxy_pass http://nextcloud.local;
    }

@mejo-
Copy link
Member Author

mejo- commented Dec 10, 2024

The PR is #6765

mejo- added a commit that referenced this issue Dec 10, 2024
mejo- added a commit that referenced this issue Dec 10, 2024
@max-nextcloud
Copy link
Collaborator

I found a way to do the redirects in vite and opened a PR that adds support for vite serve with only the following snippet left to be added to the nginx config (needs to go into the server section for nextcloud.local):

I suggest we use a non-standart port and add it to the vite config. Then we can hardcode the same port in nextcloud-docker-dev. I suggest 7347 😉

mejo- added a commit that referenced this issue Dec 11, 2024
mejo- added a commit that referenced this issue Dec 16, 2024
@mejo- mejo- closed this as completed in 5fe8c9e Dec 16, 2024
@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📝 Office team Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working javascript
Projects
Status: ☑️ Done
Development

Successfully merging a pull request may close this issue.

3 participants