Bad handle of (dynamic) optional dependencies during dependency pre-bundling

[SomaticIT]

Describe the bug

Hello,

First of all, thank you for developping this awesome tool. I use it on many projects now as the replacement of my old but stable toolchain.

Let's get into this issue: I'm maintainer of the library svelte-query which uses an optional dependency (broadcast-channel) to provide an opt-in feature.

When developers install the library on a vite project, vite is throwing an error: Failed to resolve import "broadcast-channel" (SvelteStack/svelte-query#63).

However, this dependency is imported dynamically only when the developper actually use the associated feature.

I think, there are two options to resolve this issue:

1. Delay the pre-bundling of dynamic imports when they are actually imported (best IMHO)
2. If solution 1 is not acceptable, maybe we could only delay pre-bundling of dynamic imports that are referenced in optionalDependencies in the package.json.

The only workaround I found is to install the optional dependency even if it's not used. I tried to include @sveltestack/svelte-query, broadcast-channel or both in optimizeDeps.exclude but it's not enough to workaround this issue.

I'm not familiar with vite code but I'm happy to help if needed.

Reproduction

1/ Start by creating a fresh project using create-vite:

$ npm init vite
✔ Project name: … vite-project
✔ Select a framework: › svelte
✔ Select a variant: › svelte-ts

$ cd vite-project
$ npm install

2/ Install svelte-query dependency:

$ npm install @sveltestack/svelte-query

3/ Use it in your code (eg: App.svelte):

<script lang="ts">
  // append this at the end of imports
  import { QueryClientProvider } from "@sveltestack/svelte-query";
</script>

<QueryClientProvider>
  <!-- The content of the page -->
</QueryClientProvider>

4/ Run the project:

$ npm run dev

System Info

System:
    OS: Linux 5.10 Ubuntu 20.04.3 LTS (Focal Fossa)
    CPU: (8) x64 Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
    Memory: 1.63 GB / 7.57 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
  npmPackages:
    vite: ^2.7.0 => 2.7.1

Used Package Manager

npm

Logs

[vite] Internal server error: Failed to resolve import "broadcast-channel" from "node_modules/@sveltestack/svelte-query/svelte/queryCore/broadcastQueryClient-experimental/index.js". Does the file exist?
  Plugin: vite:import-analysis
  File: /projects/vite-project/node_modules/@sveltestack/svelte-query/svelte/queryCore/broadcastQueryClient-experimental/index.js
  1  |  import '../core';
  2  |  export async function broadcastQueryClient({ queryClient, broadcastChannel = 'svelte-query', }) {
  3  |      const { BroadcastChannel } = await import('broadcast-channel');
     |                                                ^
  4  |      let transaction = false;
  5  |      const tx = (cb) => {
      at formatError (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:42587:46)
      at TransformContext.error (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:42583:19)
      at normalizeUrl (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:80909:26)
      at async TransformContext.transform (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:81049:57)
      at async Object.transform (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:42803:30)
      at async doTransform (/projects/vite-project/node_modules/vite/dist/node/chunks/dep-3daf770c.js:57478:29)

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[JackPriceBurns]

I'm having this exact same issue in a SvelteKit project!
Even in a try catch, we get the same error about vite:import-analysis

let dependency;

try {
    dependency = (await import('@org/dependency')).default;
} catch {
    //
}

We recently migrated from rollup to Vite and now our old code doesn't work which is super annoying.

[axelboc]

I'm seeing this error when conditionally importing a CSS file that doesn't exist - i.e. the error shows up and prevents compilation even when the condition is falsy:

if (import.meta.env.VITE_SOME_FALSY_VAR) {
  import('./some-css-file-that-doesnt-exist-in-this-environment.css');
}

Seems like dynamic imports are analyzed statically just like static imports.

[ZetiMente]

@SomaticIT Hi, just tried to use your repo with SvelteKit and running into this issue. Did you ever find a resolution?

[SomaticIT]

@ZetiMente: Unfortunately, I think there is no simple solution. The only way is to install optional dependencies to allow vite to analyze it. (It will not be bundled during the build)

I think, vite should only analyze and bundle dynamic dependencies when they are actually used in the code. However, it means modifying the dependency analyzer process which is a sensible part of the process.

I would be happy to take some time to create a PR (when possible) but I need some kind of help from a contributor to avoid mistakes.

[IanVS]

Hmmmm, I also need to find a way to support an optional dependency. Storybook is working on react 18 support, but in a way that also continues to support older versions. We need to dynamically import react-dom/client if using react 18, but vite throws an error since it can't resolve it in older react versions. I guess we would need to conditionally add react-dom to optimizeDeps.exclude, perhaps?

Edit: turns out I did need optimizeDeps.exclude, but also I added a conditional resolution rewrite resolveId hook of a plugin as well. Neither of these would not be in a truly dynamic situation, though, and only work because I can detect whether the import will be used at startup/build time.

[kenkunz]

I am running into this issue as well. I am trying to add an optional dependency to tradingstrategy.ai/frontend application. We would like to include an optional third-party library that's proprietary. If someone clones / forks our app who doesn't have access to the proprietary module (in a private repo within our org), we would like them to still be able to run the dev server, build, etc. We thought we'd be able to dynamically import the module inside a try / catch and gracefully degrade the UI based on whether it loaded.

The "positive" scenario works (dynamically loading when module available). The "negative" scenario results in the same vite:import-analysis error. Adding the module to optimizeDeps.exclude does not seem to help.

Let me know if there's a solution or work-around for this. Thanks!

[IanVS]

@kenkunz this is the workaround I arrived at, in addition to optimizeDeps.exclude: https://github.com/storybookjs/builder-vite/pull/320/files#diff-53261c79d121de43b9341f7d87ba28d33c2cdac416c10b0ccf5022f767937e91R68-R75.

Basically, inside a plugin, I have:

resolveId(source) {
      // Avoid error in react < 18 projects
      if (source === 'react-dom/client') {
        try {
          return require.resolve('react-dom/client', { paths: [projRoot] });
        } catch (e) {
          // This is not a react 18 project, need to stub out to avoid error
          return path.resolve(__dirname, '..', 'input', 'react-dom-client-placeholder.js');
        }
      }
    },

Where the react-dom-client-placeholder.js is an actual real file, but is empty. I get a warning about generating an empty chunk during build, but it doesn't cause any problems as far as I can tell.

[kenkunz]

Thanks for sharing this, @IanVS … I will try out this approach tomorrow!

[kenkunz]

@IanVS thanks again for your suggestion. I wound up using a different work-around – inspired by another issue you opened: #5728 [Feature request] Allow import.meta.glob from node_modules (addressed by PR #6056).

I am now loading the optional dep using:

const modules = import.meta.glob('/node_modules/chartiq/{js,css}/*.{js,css}');

…and then checking Object.keys(modules).length / importing the modules I need if available. Works great / no vite:import-analysis errors.

My only slight hesitancy with this approach is that it's a Vite-specific feature. But given that it's a work-around to a Vite-specific issue … and this is in a SvelteKit app (no real likelihood of moving away from Vite) … I'm fine with that.

[IanVS]

My workaround in #6007 (comment) is no longer working in vite 3. I notice a lot fewer dependencies are being passed to resolveId now, and my optional dep, react-dom/client, is not one of them. The error is now being thrown in vite:optimized-deps-build, I'm not sure how to work around this anymore now. :(

[patak-dev]

@IanVS would you be able to provide a minimal setup for this so we can reproduce it? Are you using enforce: 'pre' in your plugin to get before the internal resolve plugin? For reference plugin react is now using a proxy when dealing with the jsx runtime, so something, like you were doing, should continue working now too:

• vite/packages/plugin-react/src/index.ts

  Line 363 in 5151e74

  const reactJsxRuntimeId = 'react/jsx-runtime'

[IanVS]

Yes, I'm using enforce: 'pre'.

I'm sorry, I don't quite see the connection to the jsx runtime.

I'll work on a minimal reproduction.

[patak-dev]

Sorry, there is no connection to the jsx runtime. I mean that plugin-react is doing a virtual module for that dependency and only include the real one in it. A similar trick could work for you. I wonder why an alias isn't enough here, could you detect this at config time?

      // Avoid error in react < 18 projects
      if (source === 'react-dom/client') {
        try {
          return require.resolve('react-dom/client', { paths: [projRoot] });
        } catch (e) {
          // This is not a react 18 project, need to stub out to avoid error
          return path.resolve(__dirname, '..', 'input', 'react-dom-client-placeholder.js');
        }
      }

[IanVS]

Reporting back, setting an alias to point to my file seems like a usable workaround. I wasn't able to use a virtual file using the approach from 'react/jsx-runtime'. One small downside is that rollup warns about generating an empty chunk, but I can live with that.

[patak-dev]

@IanVS if you think the virtual file approach or your all workaround should work, please open a new bug report with a reproduction. I think that alias is good here, maybe we should check that warning too.

[IanVS]

I tried to create a minimal reproduction, but wasn't able to reproduce the issue when using my previous workaround without an alias on vite 3. It failed in storybook, but not when I tried to reproduce it. I'm happy to use the alias unless there's a downside to it I don't know of.

[patak-dev]

I think an alias is a better solution here, but it would be good to avoid that warning :)

[IanVS]

I can probably add a rollupOptions.onWarn to filter it out.