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

Pre-bundled dependencies doesn't dedupe imports in external files #3910

Open
6 tasks done
bluwy opened this issue Jun 22, 2021 · 10 comments
Open
6 tasks done

Pre-bundled dependencies doesn't dedupe imports in external files #3910

bluwy opened this issue Jun 22, 2021 · 10 comments
Labels
feat: deps optimizer Esbuild Dependencies Optimization has workaround p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@bluwy
Copy link
Member

bluwy commented Jun 22, 2021

Describe the bug

When Vite pre-bundles a dependency, if the dependency's entrypoint file imports an external file (e.g. Foo.svelte), imports in that external file aren't deduped.

For example, given foo-library in node_modules with these files:

// shared.js
export const bar = {}
<!-- Component.vue -->
<script>
import { bar } from './shared' // pay attention
</script>
// index.js
export { bar } from './shared'
export { default as Component } from ',/Component.vue`

Vite will prebundle foo-library as .vite/foo-library.js:

// foo-library.js
export const bar = {}
export { default as Component } from '/@fs/node_modules/foo-library/Component.vue`

^ This is the problem, Component.vue will import bar from it's relative './shared.js file, but .vite/foo-library.js has its own bar reference! This causes a lot of hard to catch bugs, especially for the Svelte ecosystem.

Reproduction

https://github.com/bluwy/vite-svelte-dedupe

(It's a Svelte-specific repro but the general issue applies)

System Info

Output of npx envinfo --system --npmPackages vite,@vitejs/plugin-vue --binaries --browsers:

  System:
    OS: Linux 5.8 Ubuntu 20.10 (Groovy Gorilla)
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
    Memory: 4.49 GB / 11.59 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.15.5 - ~/.nvm/versions/node/v14.15.5/bin/node
    Yarn: 1.22.5 - ~/.yarn/bin/yarn
    npm: 7.17.0 - ~/.nvm/versions/node/v14.15.5/bin/npm
    Watchman: 20210207.192227.0 - /usr/local/bin/watchman
  Browsers:
    Chromium: 91.0.4472.114
    Firefox: 89.0.1
  npmPackages:
    vite: ^2.3.8 => 2.3.8 

Used package manager: pnpm


Before submitting the issue, please make sure you do the following

@bluwy
Copy link
Member Author

bluwy commented Aug 3, 2021

Looking back at this, perhaps the best short-term solution for now is to not prebundle any dependencies/packages that has uncompiled files that contains JS. Basically these items:

// known SFC types
'vue',
'svelte',
'marko',
// JSX/TSX may be configured to be compiled differently from how esbuild
// handles it by default, so exclude them as well
'jsx',
'tsx',

These extensions would all technically fail, with un-deduped imports, so it's better to not do any prebundling for libraries that exports these "raw" uncompiled files.


Currently for svelte, a heuristic (package.svelte) can be used to detect when add a library to optimizeDeps.exclude. sveltejs/vite-plugin-svelte#125 may add this soon. This heuristic is also currently used in SvelteKit to add to ssr.noExternal which makes sense for uncompiled files.

However, the above isn't bullet-proof. It's best-effort only and may break depending on how one set up a project. Plus, this works for Svelte only. There's no equivalent heuristic for Vue or marko. So a built-in solution may be the best as proposed above.

@bluwy
Copy link
Member Author

bluwy commented Aug 7, 2021

To add on how snowpack/esinstall handles this, it currently bundles external files in the prebundling process as well, leading to one single bundle always.

There's of course some issues with this:

  1. We need to re-prebundle whenever the config changes. (Currently snowpack doesn't do this so prebundled bundles would get stale unless explicitly prebundled, feels like a bug)
  2. We actually need to generate two bundles, for SSR and non-SSR. Since Svelte generates different JS for these two environments. (Prebundling is browser-only)
  3. Tricky to be adopted by Vite since Vite's prebundling is done by esbuild, and Vite plugins are written in rollup object format.
  4. Tricky to handle generated CSS.

Here's also a discussion from snowpack: FredKSchott/snowpack#1808 (comment)

@bluwy
Copy link
Member Author

bluwy commented Aug 11, 2021

From some discussion lately in discord and sveltejs/vite-plugin-svelte#125, it looks that auto excluding svelte (and probably marko, solid) libraries would be the way to go, since their compiled outputs are usually only finalised when evaluated in runtime, so they can't really be pre-bundled ahead of time.

Issue

The issue now is that as @benmccann pointed out, CJS dependencies used by these excluded libraries (aka transitive CJS deps) wouldn't work in Vite, since Vite doesn't handle CJS in runtime, it expects everything to be in ESM already after prebundling, which is not the case for us. So to fix this, we have two ways:

  1. Also optimize transitive CJS deps. This makes sure that the excluded library will use ESM code from its dependencies too, essentially eliminating any CJS code imported Vite dev server.
  • Problem: This would be a breaking change for pnpm users unless they set shamefully-hoist=true or explicitly install the transitive CJS deps into the root project. This is because Vite's optimizer would look for optimized libraries under /node_modules/ only, unless if it's updated to look in nested node_modules and properly duped if needed when two excluded libraries use the same library of different major versions.
  1. Handle CJS in vite dev server. So it loads the transitive CJS deps without issues.
  • Problem: Could slow down dev server? (And probably more reasons I've not thought of)

Solution

An idea in mind is to work on no1, by altering the prebundling process to address the listed problem above. The altered flow should be like below:

  1. For normal pure JS libraries, prebundle as usual.
  2. For svelte, marko, solid (and even vue) libraries, vite will auto-exclude from optimization by default, but it'll continue to scan for imports within the library (aka deep scan).
  3. From the deep scan, for any imports found, vite will prebundle it as /node_modules/.vite/svelte-library__nested-cjs-dep.js. And make sure the library is resolved through /node_modules/svelte-library/node_modules/nested-cjs-dep, not /node_modules/nested-cjs-dep.
  4. Import analysis should correct the imports in the excluded library to use the prebundled file (/node_modules/.vite/svelte-library__nested-cjs-dep.js).

In my mind, if I'm not missing out on anything, this would work nicely with X framework libraries OOTB.

There's a small caveat however, that is these X framework libraries need to be in ESM since we exclude them from optimisation.

FAQ

Q: How do we differentiate pure JS libraries vs X framework libraries?
A: The most robust heuristic is to check if that library has .svelte or .marko components, but that's unneededly taxing on pure JS libraries. Otherwise a more lax heuristic is to check package.json for "svelte", "marko", "solid-js" dependencies (credit to @dominikg for the idea)

@patak-dev patak-dev added bug p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed pending triage labels Aug 13, 2021
@yyx990803 yyx990803 added the p4-important Violate documented behavior or significantly improves performance (priority) label Aug 13, 2021
@Shinigami92 Shinigami92 removed the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Aug 13, 2021
@bluwy
Copy link
Member Author

bluwy commented Aug 29, 2021

If anyone has stumbled on this and wants a workaround, exclude the dependency from optimization, e.g. optimizeDeps.exclude: ['the-library']. However, it could cause a side effect of #3024, if that dependency imports a CJS library. Vite 2.5.2 and above now allow us to optimize that nested CJS library, by specifying it like optimizeDeps.include: ['the-library > cjs-library'].

@benmccann
Copy link
Collaborator

There's now an experimental option to prebundle .svelte files: https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/config.md#prebundlesveltelibraries

spiffytech added a commit to spiffytech/forgo that referenced this issue Jan 13, 2022
When debugging Forgo using `npm link`, Vite's pre-bundling optimization executes the `forgo` module twice ([Vite bug](vitejs/vite#3910), [Svelte bug with workarounds](sveltejs/vite-plugin-svelte#135 (comment))). It doesn't happen when using the package from npm.

This results in different code paths holding references to two different `Fragment` symbols, causing renders to fail because the two symbols aren't comparable using `===`. Using `Symbol.for()` makes all symbols named `FORGO_FRAGMENT` comparable, no matter where they come from.

Alternatively, there's a workaround to have Vite disable pre-bundling optimizations for `forgo`, but this patch makes that unnecessary.
jeswin pushed a commit to forgojs/forgo that referenced this issue Jan 14, 2022
When debugging Forgo using `npm link`, Vite's pre-bundling optimization executes the `forgo` module twice ([Vite bug](vitejs/vite#3910), [Svelte bug with workarounds](sveltejs/vite-plugin-svelte#135 (comment))). It doesn't happen when using the package from npm.

This results in different code paths holding references to two different `Fragment` symbols, causing renders to fail because the two symbols aren't comparable using `===`. Using `Symbol.for()` makes all symbols named `FORGO_FRAGMENT` comparable, no matter where they come from.

Alternatively, there's a workaround to have Vite disable pre-bundling optimizations for `forgo`, but this patch makes that unnecessary.
@nicolas-goudry
Copy link

nicolas-goudry commented Aug 3, 2022

Hi, not sure this is related but I have a similar issue.

I’m using React 17 and use lazy-loaded routes. The issue is happening with a font awesome SVG icon.
I have two lazy-loaded routes where I use the icon in the same way:

import { faCircleQuestion } from '@fortawesome/free-solid-svg-icons/faCircleQuestion'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'

// ...

<FontAwesomeIcon icon={faCircleQuestion} />

If I first go to route A, the icon works. If I first go to route B, the icon doesn’t work. But, if I first go to route A, then route B, the icon works!

I looked at the code generated after building the app, and I can see this in route B’s JS files:

/* file1.js */
// ...

var le = {};

// ...

export {le as f};

/* file2.js */
import {f as ma} from "./file1.js";

// ...

children: c(Xn, {
  icon: ma.faCircleQuestion
})

As you can see, file1.js creates a le variable containing an empty object and exports it as f. file2.js imports f as ma and then tries to use the faCircleQuestion property of ma. Since ma is an empty object, the icon definition is not passed to the FontAwesomeIcon component, and nothing renders.

I tried to add @fortawesome/free-solid-svg-icons in optimizeDeps.exclude field of my config, but nothing changed. The only way I was able to make it work on both routes first load was to import the definition property of the icon (only in route B!) rather than its alias:

/* unbuilded code */
import { definition as faCircleQuestion } from '@fortawesome/free-solid-svg-icons/faCircleQuestion'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'

// ...

/* file1.js */
// ...

var N = {};
(function(e) {
    Object.defineProperty(e, "__esModule", {
        value: !0
    });
    var n = "fas"
      , a = "circle-question"
      , i = 512
      , r = 512
      , t = [62108, "question-circle"]
      , o = "f059"
      , l = "M256 0C114.6 0 0 114.6 0 256s114.6 256 256 256s256-114.6 256-256S397.4 0 256 0zM256 400c-18 0-32-14-32-32s13.1-32 32-32c17.1 0 32 14 32 32S273.1 400 256 400zM325.1 258L280 286V288c0 13-11 24-24 24S232 301 232 288V272c0-8 4-16 12-21l57-34C308 213 312 206 312 198C312 186 301.1 176 289.1 176h-51.1C225.1 176 216 186 216 198c0 13-11 24-24 24s-24-11-24-24C168 159 199 128 237.1 128h51.1C329 128 360 159 360 198C360 222 347 245 325.1 258z";
    e.definition = {
        prefix: n,
        iconName: a,
        icon: [i, r, t, o, l]
    },
    e.faCircleQuestion = e.definition,
    e.prefix = n,
    e.iconName = a,
    e.width = i,
    e.height = r,
    e.ligatures = t,
    e.unicode = o,
    e.svgPathData = l,
    e.aliases = t
}
)(N);

// ...

export {N as f};

/* file2.js */

// content unchanged

This way, it works even if I first load route B, and still works when I first load route A.

I might add that this is not happening with development server, only when builded and served by a simple nginx server.

EDIT: I also was able to make it work by updating both routes code like this:

import faCircleQuestion from '@fortawesome/free-solid-svg-icons/faCircleQuestion'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'

// ...

<FontAwesomeIcon icon={faCircleQuestion.faCircleQuestion} />

@ZengTianShengZ
Copy link

ZengTianShengZ commented Sep 2, 2022

Is this problem solved? I have a similar problem

The vite dev environment is packaged with two versions, which causes the configuration to be loaded separately, and the instance cannot be shared.

http://localhost:8082/@fs/Users/xxxxxx/node_modules/@xxxx/config.js';

and

http://localhost:8082/node_modules/@xxxx/config.js';

I have this problem in [email protected], but upgrading to [email protected] has not solved it.

@bluwy
Copy link
Member Author

bluwy commented Sep 4, 2022

@ZengTianShengZ this isn't fixed yet, but you can workaround by adding the dependency to optimizeDeps.exclude

@Flight
Copy link

Flight commented Oct 11, 2023

Already spent a few weeks on my project fighting that bug. We are using vite + pnpm and a lot of third-party deps in our internal library (in our artefacts repo), so the list of the includes is already like this:

optimizeDeps: {
  include: [
      "recharts"
      "prop-types",
      "react-is",
      "react-dom",
      "hoist-non-react-statics",
      "react/jsx-runtime",
      // Please add the icons you need here
      "@mui/icons-material/Search",
      "@mui/icons-material/ExpandMore",
      "@mui/icons-material/InfoOutlined",
      "@mui/icons-material/Info",
      "@mui/icons-material/ArrowUpwardOutlined",
      "@mui/icons-material/RemoveOutlined",
      "@mui/icons-material/PsychologyOutlined",
      "@mui/icons-material/MenuBookOutlined",
      "@mui/icons-material/GroupsOutlined",
      "@mui/icons-material/ScatterPlotOutlined",

      .....
  ]
}

The developers are super tired of adding each dependency one by one including each icon! And the problem is that you are finding out that the dependency is missing only after you pull the latest version of the lib from the artifacts report and trying to use it 😞
Do you have any advice on some bundler with a similar speed to replace Vite where it works out of the box without all those hacks?

@bluwy
Copy link
Member Author

bluwy commented Oct 11, 2023

I'm not sure this issue is related to what you're getting. You can check https://vitejs.dev/guide/dep-pre-bundling.html#monorepos-and-linked-dependencies if you're working in a monorepo.

Otherwise for your case in particular, perhaps you can try @mui/icons-material/* too.

@patak-dev patak-dev removed the bug label Feb 10, 2024
westonpace added a commit to lancedb/lancedb that referenced this issue Mar 4, 2024
Arrow-js uses brittle `instanceof` checks throughout the code base.
These fail unless the library instance that produced the object matches
exactly the same instance the vectordb is using. At a minimum, this
means that a user using arrow version 15 (or any version that doesn't
match exactly the version that vectordb is using) will get strange
errors when they try and use vectordb.

However, there are even cases where the versions can be perfectly
identical, and the instanceof check still fails. One such example is
when using `vite` (e.g. vitejs/vite#3910)

This PR solves the problem in a rather brute force, but workable,
fashion. If we encounter a schema that does not pass the `instanceof`
check then we will attempt to sanitize that schema by traversing the
object and, if it has all the correct properties, constructing an
appropriate `Schema` instance via deep cloning.
raghavdixit99 pushed a commit to raghavdixit99/lancedb that referenced this issue Apr 5, 2024
Arrow-js uses brittle `instanceof` checks throughout the code base.
These fail unless the library instance that produced the object matches
exactly the same instance the vectordb is using. At a minimum, this
means that a user using arrow version 15 (or any version that doesn't
match exactly the version that vectordb is using) will get strange
errors when they try and use vectordb.

However, there are even cases where the versions can be perfectly
identical, and the instanceof check still fails. One such example is
when using `vite` (e.g. vitejs/vite#3910)

This PR solves the problem in a rather brute force, but workable,
fashion. If we encounter a schema that does not pass the `instanceof`
check then we will attempt to sanitize that schema by traversing the
object and, if it has all the correct properties, constructing an
appropriate `Schema` instance via deep cloning.
raghavdixit99 pushed a commit to raghavdixit99/lancedb that referenced this issue Apr 5, 2024
Arrow-js uses brittle `instanceof` checks throughout the code base.
These fail unless the library instance that produced the object matches
exactly the same instance the vectordb is using. At a minimum, this
means that a user using arrow version 15 (or any version that doesn't
match exactly the version that vectordb is using) will get strange
errors when they try and use vectordb.

However, there are even cases where the versions can be perfectly
identical, and the instanceof check still fails. One such example is
when using `vite` (e.g. vitejs/vite#3910)

This PR solves the problem in a rather brute force, but workable,
fashion. If we encounter a schema that does not pass the `instanceof`
check then we will attempt to sanitize that schema by traversing the
object and, if it has all the correct properties, constructing an
appropriate `Schema` instance via deep cloning.
westonpace added a commit to lancedb/lancedb that referenced this issue Apr 5, 2024
Arrow-js uses brittle `instanceof` checks throughout the code base.
These fail unless the library instance that produced the object matches
exactly the same instance the vectordb is using. At a minimum, this
means that a user using arrow version 15 (or any version that doesn't
match exactly the version that vectordb is using) will get strange
errors when they try and use vectordb.

However, there are even cases where the versions can be perfectly
identical, and the instanceof check still fails. One such example is
when using `vite` (e.g. vitejs/vite#3910)

This PR solves the problem in a rather brute force, but workable,
fashion. If we encounter a schema that does not pass the `instanceof`
check then we will attempt to sanitize that schema by traversing the
object and, if it has all the correct properties, constructing an
appropriate `Schema` instance via deep cloning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization has workaround p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

No branches or pull requests

9 participants