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.client doesn't care about previous chunks when the update of the app. #7226

Open
genki opened this issue Jan 4, 2025 · 30 comments
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working

Comments

@genki
Copy link
Contributor

genki commented Jan 4, 2025

Which component is affected?

Qwik Rollup / Vite plugin

Describe the bug

I am writing somewhat large app with Qwik and when I tried to update it, the pnpm run build.client generates about 500-600 chunks but they have almost different hash from the previous output.

When I would take a rolling update of the app, the existing app running in the browser experiences failure of dynamic import against the previous chunks, because they are no longer exist at that place after the update.

I wanted to make the experience seamless during the app update.
How can I include the previous chunks into the newly generated ones and merge the newer and older q-manifest?

[Updated] I added a reproduction.

Reproduction

https://github.com/genki/qwik-test/tree/fragile_hash_test

Steps to reproduce

Please clone the repo above and install packages then run many times (I expect 10〜20 times)

pnpm run build.client

Please looking into the hash of the last chunk, the largest file.
Because of this reproduction is very smaller than the real case, the result is mostly same hash.
But sometimes it changes, even if no code change has included.

I have attached the case of generated different bundle at under the ./tmp

https://github.com/genki/qwik-test/tree/fragile_hash_test/tmp

Those 2 files are the largest chunks from different 2 builds that are from the same source without any modification.
They are mostly same, but there are small diff like this:

4934c4934
< const Layout = () => __vitePreload(() => import("./q-VBnW36aw.js"), true ? [] : void 0);
---
> const Layout = () => __vitePreload(() => import("./q-ywWxxcYW.js"), true ? [] : void 0);
4939c4939
< const e = () => __vitePreload(() => import("./q-BNyqOpV5.js"), true ? [] : void 0);
---
> const e = () => __vitePreload(() => import("./q-C0bxF2sP.js"), true ? [] : void 0);
5311,5313d5310
< };
< const s_VKFlAWJuVm8 = () => {
<   return /* @__PURE__ */ _jsxC(Slot, null, 3, "yB_0");
5350a5348,5350
> const s_VKFlAWJuVm8 = () => {
>   return /* @__PURE__ */ _jsxC(Slot, null, 3, "yB_0");
> };
5377,5378c5377,5378
<   s_VKFlAWJuVm8 as x,
<   s_B0lqk5IDDy4 as y,
---
>   s_B0lqk5IDDy4 as x,
>   s_VKFlAWJuVm8 as y,

System Info

System:
    OS: macOS 14.6.1
    CPU: (8) arm64 Apple M2
    Memory: 75.95 MB / 24.00 GB
    Shell: 3.6.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 20.18.1 - /opt/homebrew/opt/node@20/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 9.9.4 - ~/project/jotter/node_modules/.bin/npm
    pnpm: 9.11.0 - /opt/homebrew/bin/pnpm
    bun: 1.1.26 - ~/.bun/bin/bun
  Browsers:
    Chrome: 131.0.6778.205
    Safari: 17.6
  npmPackages:
    @builder.io/qwik: file:../clone/qwik/packages/qwik => 1.10.0 
    @builder.io/qwik-city: file:../clone/qwik/packages/qwik-city => 1.10.0 
    typescript: ^5.4.5 => 5.4.5 
    undici: ^5.28.4 => 5.28.4 
    vite: ^5.4.11 => 5.4.11

Additional Information

In addition, when the app size is larger, the pnpm run build.client generates almost different hashes even if there's no changes in the source code. Is this expected behaviour?

@genki genki added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Jan 4, 2025
@gioboa
Copy link
Member

gioboa commented Jan 5, 2025

Hi @genki with the same code the hash should remain the same. If not we need to figure out the why. If you need to preserve the old chunks too you can implement a Vite plugin that read and write them somewhere to preserve them during builds. Btw this could be a Qwik community plugin.

@gioboa
Copy link
Member

gioboa commented Jan 5, 2025

Another solution cloud be: a Vite plugin that download all the current chunks locally ( based on q-manifest ) and merge them after the build process

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

@gioboa Thank you for the answer. I wonder why the change happens to q-manifest.json and chunks even if no updates in my source code. Of course, no dynamic define variables in the vite.config.ts
Here's the example,

> pnpm run build.client; cp dist/q-manifest.json > ./q-manifest1.json
> pnpm run build.client; cp dist/q-manifest.json > ./q-manifest2.json
> diff q-manifest1.json q-manifest2.json | wc
   29908   63910  927981

There are many chunks built, but most of them are very small and only consists of imports other chunks and one export like this.

import"./q-CLq9cP0Z.js";import{c as N}from"./q-TmSm9_Ab.js";import"./q-iUhm9qIU.js";import"./q-CDfZr0w7.js";import"./q-BVGg7BBr.js";import"./q-CT7XUfUO.js";export{N as s_BdTzVN1StNM};

And these chunks that are imported here are also changed while the build 1 and 2.

As I understood this is something wrong I would investigate more.
Thanks :)

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

Ah, I have one more question. Such the piles of small code snippets that are consist of only imports and one export is expected behaviour?.

✓ 1367 modules transformed.
dist/qwik-prefetch-service-worker.js     3.91 kB
dist/build/q-bundle-graph-l4ai7h.json   11.18 kB │ gzip:   4.97 kB
dist/q-manifest.json                   537.54 kB │ gzip:  65.24 kB
dist/build/q-BvjWe9F_.js                 0.07 kB │ gzip:   0.07 kB
dist/build/q-CogWlRVA.js                 0.07 kB │ gzip:   0.07 kB
dist/build/q-BMYaqiyb.js                 0.07 kB │ gzip:   0.07 kB
dist/build/q-2Ng8QIWW.js                 0.08 kB │ gzip:   0.08 kB
dist/build/q-KlVLRa-A.js                 0.09 kB │ gzip:   0.09 kB
dist/build/q-C-FxUaGa.js                 0.09 kB │ gzip:   0.09 kB
dist/build/q-DmmqG_DK.js                 0.09 kB │ gzip:   0.09 kB
dist/build/q-DVcyZDSr.js                 0.10 kB │ gzip:   0.10 kB
dist/build/q-Dj5oz7D0.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-BIBeDro2.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-CyIYmTQF.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-Nzln8svg.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-CIQ4usUd.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-BeKq1wWa.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-DVsNUtgV.js                 0.12 kB │ gzip:   0.11 kB
dist/build/q-OrzVETKF.js                 0.12 kB │ gzip:   0.11 kB
...

I don't provide any customization to rollup options. vite.config.ts is almost plain just the size of the app are a bit large.
I wonder if that the too finer granularity of chunks draws inefficiency. Is this the usual output from the smart entry strategy?

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

Here is my vite.config.ts that causes this issue.

const WIDTHS = [640, 1024];
export default defineConfig(():UserConfig => {
  return {
    plugins: [
      qwikCity(),
      qwikVite(),
      tsconfigPaths(),
    ],
    ssr: {
      external: ['node:async_hooks'],
    },
    preview: {
      headers: {
        "Cache-Control": "public, max-age=600",
        "X-Content-Type-Options": "nosniff",
      },
    },
    resolve: {
      alias: {
        "~": path.resolve(__dirname, "src"),
      },
    },
    css: {
      preprocessorOptions: {
        sass: {
          additionalData: `$widths: ${WIDTHS.map(w => w + "px").join(",")}\n`,
        },
      },
    },
  };
});

@gioboa
Copy link
Member

gioboa commented Jan 5, 2025

I guess so, can you try to create a small reproduction repo to figure out the situation and solve the issue please?

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

@gioboa Yeah, I think so too. I have tried with no luck. If get success, I will come back :)

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

Further investigation about the output, I found the cyclic import between RouterOutlet and spaInit in a chunk maybe regarding to the QwikCity.

Chunk: q-D9K9vuh1.js

const RouteStateContext = /* @__PURE__ */ createContextId("qc-s");
const ContentContext = /* @__PURE__ */ createContextId("qc-c");
const ContentInternalContext = /* @__PURE__ */ createContextId("qc-ic");
const DocumentHeadContext = /* @__PURE__ */ createContextId("qc-h");
const RouteLocationContext = /* @__PURE__ */ createContextId("qc-l");
const RouteNavigateContext = /* @__PURE__ */ createContextId("qc-n");
const RouteActionContext = /* @__PURE__ */ createContextId("qc-a");
const RouteInternalContext = /* @__PURE__ */ createContextId("qc-ir");
const RoutePreventNavigateContext = /* @__PURE__ */ createContextId("qc-p");
const spaInit = eventQrl(/* @__PURE__ */ qrl(() => __vitePreload(() => import("./q-4Y1X7MJF.js"), true ? [] : void 0), "s_o0fE7sdgJro"));
const RouterOutlet = /* @__PURE__ */ componentQrl(/* @__PURE__ */ qrl(() => __vitePreload(() => import("./q-C6SJWZkU.js"), true ? [] : void 0), "s_0rmCF5rEoas"));

The last line dynamic imports the RouterOutlet. The imported code is importing it back circulary at the 1st line.

Chunk: q-C6SJWZkU.js

import { B as spaInit, d as ContentInternalContext } from "./q-D9K9vuh1.js";
import { a as useServerData, C as _jsxBranch, u as useContext, l as _jsxC, f as _jsxQ, p as _qrlSync, F as Fragment, D as SkipRender } from "./q-Bf_tcDXr.js";
const s_0rmCF5rEoas = () => {
  const serverData = useServerData("containerAttributes");
  if (!serverData) throw new Error("PrefetchServiceWorker component must be rendered on the server.");
  _jsxBranch();
  const context = useContext(ContentInternalContext);
  if (context.value && context.value.length > 0) {
    const contentsLen = context.value.length;
    let cmp = null;
    for (let i = contentsLen - 1; i >= 0; i--) if (context.value[i].default) cmp = _jsxC(context.value[i].default, {
      children: cmp
    }, 1, "Ms_0");
    return /* @__PURE__ */ _jsxC(Fragment, {
      children: [
        cmp,
        /* @__PURE__ */ _jsxQ("script", {
          "document:onQCInit$": spaInit,
          "document:onQInit$": _qrlSync(() => {
            ((w, h) => {
              var _a;
              if (!w._qcs && h.scrollRestoration === "manual") {
                w._qcs = true;
                const s = (_a = h.state) == null ? void 0 : _a._qCityScroll;
                if (s) w.scrollTo(s.x, s.y);
                document.dispatchEvent(new Event("qcinit"));
              }
            })(window, history);
          }, '()=>{((w,h)=>{if(!w._qcs&&h.scrollRestoration==="manual"){w._qcs=true;const s=h.state?._qCityScroll;if(s){w.scrollTo(s.x,s.y);}document.dispatchEvent(new Event("qcinit"));}})(window,history);}')
        }, null, null, 2, "Ms_1")
      ]
    }, 1, "Ms_2");
  }
  return SkipRender;
};
export {
  s_0rmCF5rEoas
};

At least those two chunks can't be exist stably because the hash can't be computed.
The one needs the finished code to compute the hash while the other one too needs the finished code of the first one to compute the hash.

I guess the Vite would probably roughly solve this deadlock by choosing a random hash for such the chunks.

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

The why only the larger app cause this issue is maybe those two chunks are not splitted in a smaller apps.
To solve this issue, chunks that have circular imports regardless that it is static or dynamic, they should be bundled in one chunk.

@genki
Copy link
Contributor Author

genki commented Jan 5, 2025

Relating to vitejs/vite#10636

@gioboa
Copy link
Member

gioboa commented Jan 5, 2025

The last line dynamic imports the RouterOutlet. The imported code is importing it back circulary at the 1st line.

We definitely need to solve this

cc @wmertens

@genki
Copy link
Contributor Author

genki commented Jan 6, 2025

@gioboa @wmertens
I know the rollup has the algorithm that can solve the circular import problem after the version 3.0, but it is not perfect and in this case fails for resolving.

But finally, I have found the threshold of the fail and success.
That was use of the nonce for CSP. If I removed the code brought from here https://qwik.dev/docs/advanced/content-security-policy/#content-security-policy the hash values became stable.

Alas, the nonce is random value.
How can I remove it from the generated bundle and keep the CSP safe?
One idea is injecting the value at the adapters.

@genki
Copy link
Contributor Author

genki commented Jan 6, 2025

@genki
Copy link
Contributor Author

genki commented Jan 6, 2025

At any rate, the nonce is not the only reason of this issue.
For example, changing the use of routeLoader$ is sometimes affect to the fundamental result of the output and changes a lot of hashes.
The stability of the cache is very fragile against the change of the code.
I think there should be chunk management system between the old and new build for the application update.

@genki
Copy link
Contributor Author

genki commented Jan 6, 2025

I wrote a vite pluting that merges previous build into ./dist/build/* prior to the new build.
It expects to keep the previous built at ./current.
It's simple but worked well at my environment :)

import type {Plugin} from 'vite';
import fs from 'fs';
import path from 'path';

const CUR_BUILD = path.resolve(process.cwd(), 'current');

type Options = {
  expireDuration?: number; // in seconds
}

export function mergeBuild({expireDuration}:Options = {}): Plugin {
  const now = Date.now();
  let count = 0;
  let size = 0;
  return {
    name: 'merge-build',
    enforce: 'pre',
    generateBundle: {
      order: "pre",
      handler() {
        console.log('merge-build copying files...');
        const dist = path.resolve(process.cwd(), 'dist');
        if (!fs.existsSync(dist)) fs.mkdirSync(dist);
        const distBuild = path.resolve(dist, 'build');
        if (!fs.existsSync(distBuild)) fs.mkdirSync(distBuild);

        // copy current build into distBuild dir wiout change ctime/mtime
        const curBuild = fs.readdirSync(CUR_BUILD);
        for (const file of curBuild) {
          const src = path.resolve(CUR_BUILD, file);
          const dst = path.resolve(distBuild, file);
          if (fs.existsSync(dst)) continue;
          const stats = fs.statSync(src);
          // check if file is not expired
          if (expireDuration !== undefined) {
            if (now - stats.mtimeMs > expireDuration * 1000) continue;
          }
          fs.copyFileSync(src, dst);
          // copy ctime/mtime
          fs.utimesSync(dst, stats.atime, stats.mtime);
          count++;
          size += stats.size;
        }
      }
    },
    closeBundle() {
      const prettySize = (size / 1024).toFixed(2) + 'kB';
      console.log("merge-build:", count, "files", prettySize,
        "merged from previous build");
    }
  };
}

@gioboa
Copy link
Member

gioboa commented Jan 6, 2025

Thanks, that's nice 🥳
Would you like to create a cookbook example in the qwik docs https://qwik.dev/docs/cookbook/ ?
I'm wondering how modify this plugin to make it usable in a CI/CD pipeline 🤔

@genki
Copy link
Contributor Author

genki commented Jan 7, 2025

Sorry I am not good at natural languages other than Japanese.
I would like to concentrate to write some code

@gioboa
Copy link
Member

gioboa commented Jan 7, 2025

Ok, can you try to modify the plugin to work on CI/CD pipeline env? ( reading and fetching the production chunks )

@genki
Copy link
Contributor Author

genki commented Jan 8, 2025

@gioboa
As I am not using CI/CD pipeline actively so I am not familiar with, but isn't it necessary additional storage such as S3, Google Cloud Storage?
If there is, the modification is simply, push dist/build/* to the storage after the build, and fetch them into ./current before the next build, keeping their ctime/mtime of the files.

@wmertens
Copy link
Member

wmertens commented Jan 8, 2025

Wow good research in this issue. Another thing that changes the hashes is terser, because it picks variable names based on frequency, which changes a lot of code.

See vitejs/vite#17589

In vite 6 you can now configure terser to use a static set.

@gioboa
Copy link
Member

gioboa commented Jan 8, 2025

If there is, the modification is simply, push dist/build/* to the storage after the build, and fetch them into ./current before the next build, keeping their ctime/mtime of the files.

That's a good idea, is there a free service we can add at that example?

@genki
Copy link
Contributor Author

genki commented Jan 8, 2025

@gioboa I too wanted to know the free storage.
Another idea is prepare a dedicated repository on GitHub and commit the build artifacts to and pull them out when the next time.

@wmertens Indeed.
I think making the hashes stable over the dependency tree is a difficult problem because it is almost same structure to the merkle tree which is designed to be as fragile as possible against any changes.
To achieve the stability, it is need to split entire code into chunks that have no dependencies each other.

I think the root of this problem is misunderstand about the dynamic import.
The dynamic import can solve the circular dependency problem at runtime, but computing the hashes is happen at the build time so thus that is no help.
To achieve hash stability, the dynamic imports should not use the hash but use the stable name with Etag/last-modified response header.

const {...} = await import(`/name_of_module`)

@gioboa
Copy link
Member

gioboa commented Jan 8, 2025

In vite 6 you can now configure terser to use a static set.

Well, Vite 6 is out so let try to fix this problem

@genki
Copy link
Contributor Author

genki commented Jan 15, 2025

@gioboa Is Qwik is ready to be used with the Vite@6? The package.json is pointing to Vite@5.

@gioboa
Copy link
Member

gioboa commented Jan 15, 2025

@gioboa Is Qwik is ready to be used with the Vite@6? The package.json is pointing to Vite@5.

Yep, it's working fine with Vite v6 🚀

@genki
Copy link
Contributor Author

genki commented Jan 16, 2025

@gioboa
Thanks. I've tested with the Vite@6, the generated bundles are stay randomly changes.

@wmertens
Do you know how to configure the static set to the terser?

@gioboa
Copy link
Member

gioboa commented Jan 16, 2025

I think is this one build terser

I found this from terser repo

@genki
Copy link
Contributor Author

genki commented Jan 17, 2025

@gioboa Thanks! I will check it out :)

@genki
Copy link
Contributor Author

genki commented Jan 18, 2025

Finally, I could add the reproduction.
I have simulated the large app by specifying entryStrategy: {type:"single"} to enlarge the largest chunk.
The repo is very small and simple but it causes the difference on build artifact sometimes if run many times pnpm run build.client.
It's rare though this example is small, but for the real larger app this issue always happens.

@genki
Copy link
Contributor Author

genki commented Jan 18, 2025

I think there is unexpected randomness in the building process.
Looking into this lines in the diff,

5377,5378c5377,5378
<   s_VKFlAWJuVm8 as x,
<   s_B0lqk5IDDy4 as y,
---
>   s_B0lqk5IDDy4 as x,
>   s_VKFlAWJuVm8 as y,

the order of symbols is different. There can be the unconscious race conditions in the vite/rollup?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants