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

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

Closed
shairez opened this issue Sep 4, 2023 · 8 comments
Assignees
Labels
TYPE: bug Something isn't working

Comments

@shairez
Copy link
Contributor

shairez commented Sep 4, 2023

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:
qwikifiers/qwik-ui#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)

@shairez shairez added TYPE: bug Something isn't working STATUS-1: needs triage New issue which needs to be triaged WAITING FOR: team Waiting for one of the core team members to review and reply and removed STATUS-1: needs triage New issue which needs to be triaged labels Sep 4, 2023
@mhevery
Copy link
Contributor

mhevery commented Sep 5, 2023

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.)

@mhevery mhevery removed the WAITING FOR: team Waiting for one of the core team members to review and reply label Sep 5, 2023
@mhevery mhevery self-assigned this Sep 5, 2023
@shairez
Copy link
Contributor Author

shairez commented Sep 9, 2023

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
Copy link
Contributor

mhevery commented Sep 11, 2023

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

@maiieul
Copy link
Contributor

maiieul commented Nov 19, 2023

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 qwikifiers/qwik-ui#519

@maiieul
Copy link
Contributor

maiieul commented Nov 20, 2023

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:

  • pnpm install
  • pnpm preview
  • global search for "I am a duplicate" (make sure to not exclude settings and ignore files)
  • global search for the export as symbol (in the form of s_T0D1wZ7CkuM)
  • See that it is placed in the same entry_AccordionRoot file.

image

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

@mhevery
Copy link
Contributor

mhevery commented Dec 10, 2023

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

@maiieul
Copy link
Contributor

maiieul commented Dec 10, 2023

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
Copy link
Contributor Author

shairez commented Dec 11, 2023

I agree with @maiieul

So I opened a new issue #5559 to be more specific
and I'm closing this one

@shairez shairez closed this as completed Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TYPE: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants