QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.83k stars 1.3k forks source link

[🐞] Tree shaking doesn't work for qwik libraries using the recommended build script #5097

Closed shairez closed 11 months ago

shairez commented 1 year ago

Which component is affected?

Qwik Rollup / Vite plugin

Describe the bug

Out of the box, Qwik's build script for the "library" starter (component library for example) produces one big index.qwik.mjs (and .cjs) containing ALL of the library code.

When consuming the library from a qwik app project, ALL of the library is getting built into one big chunk and getting prefetched (and not only the used code from that library).

Here's a real life example of this bug: https://github.com/qwikifiers/qwik-ui/issues/384

POSSIBLE SOLUTIONS:

  1. Fix the tree shaking so it'll only consume the imported parts from the index.qwik.mjs

  2. Change the default way libs are getting built in by outputting individual js files for each library file (instead of a giant index file)

Reproduction

https://github.com/shairez/qwik-ui-headless-import-issue-384

Steps to reproduce

  1. clone the repo
  2. Run pnpm preview
  3. Check the network tab
  4. Disabled Cache
  5. Refresh the page
  6. See the entire qwik ui headless library getting consumed while only the Accordion was used.

image

image

System Info

System:
    OS: Linux 5.15 Ubuntu 20.04.6 LTS (Focal Fossa)
    CPU: (5) x64 Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
    Memory: 14.07 GB / 23.48 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 18.16.1 - ~/.volta/tools/image/node/18.16.1/bin/node
    Yarn: 4.0.0-rc.46 - ~/.volta/tools/image/yarn/4.0.0-rc.46/bin/yarn
    npm: 9.5.1 - ~/.volta/tools/image/node/18.16.1/bin/npm
    pnpm: 8.6.7 - ~/.volta/bin/pnpm
  Browsers:
    Chrome: 114.0.5735.133
  npmPackages:
    @builder.io/qwik: ^1.2.10 => 1.2.10
    @builder.io/qwik-city: ^1.2.10 => 1.2.10
    undici: 5.22.1 => 5.22.1
    vite: 4.4.7 => 4.4.7

Additional Information

POSSIBLE SOLUTIONS:

  1. Fix the tree shaking so it'll only consume the imported parts from the index.qwik.mjs

  2. Change the default way libs are getting built in by outputting individual js files for each library file (instead of a giant index file)

mhevery commented 1 year ago

Argh! That is a good one!

I looked into why all of the symbols are retained, and it is because they are all referenced, so the rollup is doing the right thing. The question is, why are they all referenced?

The issue is that qwik-ui library has components, and each component has a listener hence a QRL. Qwik does not know if any one QRL is being used so it sides on the side of caution and generates entry points for them anyway. But those entry points end up pulling in all of the qwik-ui.

So this is a Qwik optimizer issue (not a mistake in how qwik-ui has been bundled.)

shairez commented 1 year ago

update: tried to change the config to "preserve modules" and output a file for each source file

Didn't work, it fails on loading some qrls now

should I create a different issue? or just a detailed comment here?

mhevery commented 1 year ago

I think it is related so just add it here....

maiieul commented 12 months ago

Using preserveModules and preserveModulesroot seems to be getting rid of the big bundle file, as it transfers the bundling to the consumer app, but the only problem is that it creates namespace issues.

So using

    rollupOptions: {
      output: {
        preserveModules: true,
        preserveModulesRoot: 'packages/kit-headless/src',
      },
    },

if the library has a component$ named AccordionRoot, and then I define export const AccordionRoot = component$(...) in the consumer app, the AccordionRoot component from the library will be overridden by the AccordionRoot in the consumer app, leading to "Cannot resolve symbol s_c7PiYJ1s0GU... Error code (31)" types of errors when symbols are trying to reach the overridden AccordionRoot code as it's not available anymore.

Maybe we could resolve those namespace conflicts with the optimizer? And then have preserveModules and preserveModulesroot as the default vite.config for library mode? I'm not sure this would be an easier fix, but this is worth considering. We could have a different hash for the entry file as it's already being done when there are multiple layouts and index files in the routes folder.

P.S. To see the AccordionRoot entry file being overridden I set the following vite config:

import { defineConfig } from "vite";
import { qwikVite } from "@builder.io/qwik/optimizer";
import { qwikCity } from "@builder.io/qwik-city/vite";
import tsconfigPaths from "vite-tsconfig-paths";

export default defineConfig(() => {
  let rollupOptions = {};

  if (process.env.npm_lifecycle_event === "build.client") {
    // Client-specific configuration
    rollupOptions = {
      output: {
        // Customize the client build structure
        entryFileNames: (fileInfo:any) => {
          return `build/[name]-[hash].js`
        },
        chunkFileNames: (fileInfo:any) => {
          return `build/[name]-[hash].js`
        },
        assetFileNames: `build/[name]-[hash].[ext]`,
      },
    };
  }

  return {
    build: {
      rollupOptions,
    },
    preview: {
      headers: {
        "Cache-Control": "public, max-age=600",
      },
    },
    plugins: [qwikCity(), qwikVite(), tsconfigPaths()],
  };
});

It outputs the build files as

dist/build/entry_Accordion-f2e09d9d.js
dist/build/entry_AccordionContent-ccfa0f47.js
dist/build/entry_AccordionItem-2aedcde9.js
dist/build/entry_AccordionRoot-40cd33d0.js
dist/build/entry_AccordionTrigger-9c3d5364.js

instead of

dist/build/q-2eb0621e.js
dist/build/q-2ee379bd.js
dist/build/q-3bc22926.js
dist/build/q-3bdb1444.js
dist/build/q-3fae3359.js

P.S. #2: Repro and steps in https://github.com/qwikifiers/qwik-ui/issues/519

maiieul commented 11 months ago

FWIW, if I pnpm uninstall the library and copy/paste the code, the error is gone, but then I lose the benefits of using a library.

The interesting bit is that when I have a duplicate component in a Qwik project (that is not coming from an external library), the optimizer creates two symbols for the components and attaches them to the same entry.

Repro: https://github.com/maiieul/qwik-component-duplicate

I'm using preserveModules in this repro to see the symbols files in the build output.

Steps:

image

I can create another issue with this repro if this is of concern.

mhevery commented 11 months ago

Hey @shairez I believe this has been solved. Can we close this?

maiieul commented 11 months ago

I'm not sure this can be marked as fixed unless we add something like

      output: {
        preserveModules: true,
        preserveModulesRoot: '/src',
      },

to the default vite.config of the library mode starter of npm create qwik@latest

The problem is that preserveModules introduces the namespace issues of #5473.

shairez commented 11 months ago

I agree with @maiieul

So I opened a new issue https://github.com/BuilderIO/qwik/issues/5559 to be more specific and I'm closing this one