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-bundling should be more resilient to errors #3715

Closed
Conduitry opened this issue Jun 8, 2021 · 5 comments
Closed

Pre-bundling should be more resilient to errors #3715

Conduitry opened this issue Jun 8, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@Conduitry
Copy link

Clear and concise description of the problem

When Vite fails during the pre-bundling of a particular dependency for the browser, it seems to get into a state where it refuses to pre-bundle any further dependencies. Even if the reference to the offending dependency is removed, in dev mode Vite remains in this state until the server is restarted.

This is an issue because some dependencies simply can't be pre-bundled for the browser (for example, if they make use of Node APIs). Mentioning these in code that's meant to be isomorphic for the browser and the server - even if it's inside if (false) { import ('something'); } - causes Vite to attempt to pre-bundle them, resulting in an error, and also resulting in other dependencies not being pre-bundled, resulting in (for example) CJS being served to the browser when it's expecting ESM.

Suggested solution

I do not know Vite's internals well enough to suggest what should be changed.

Alternative

Other options that seem less good to me (because they don't solve the underlying brittleness issue) would be to make Vite smarter about import()s that it sees could never be reached, or to make Vite only actually bundle dependencies for the browser when they are requested. Both of these might be good ideas by themselves, but even if someone does accidentally import something on the browser that cannot be bundled for the browser, this shouldn't prevent other dependencies from being pre-bundled until Vite is restarted.

Additional context

See sveltejs/kit#1570 which contains a reproduction using SvelteKit.

For now, my workaround is to hide the import()s from Vite that I don't want it to try to pre-bundle by wrapping them in another function call, which then calls import().

@AndreasHald
Copy link

We experience this issue aswell, when updating from 2.3.0 -> 2.5.7 (through svelte-kit) most dependencies that were previously only exposed to the server is now exposed to the browser. And everything fails.

@bluwy
Copy link
Member

bluwy commented Sep 28, 2021

@AndreasHald Could be a vite-plugin-svelte issue, what dependency in particular that is causing that? disableDependencyReinclusion might help too.

@bluwy bluwy added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) and removed enhancement: pending triage labels Mar 11, 2022
@alexlafroscia
Copy link

alexlafroscia commented Apr 22, 2022

I'm running into a similar issue related to an attempt at client-bundling an unnecessary package.

In my case, I have two packages in a mono-repo:

  • A library that exports two functions; validate and request
  • a SvelteKit app (which maybe complicates things?) that consumes this library

validate is totally safe to run on the server and client, and it's imported and used in both.

request wraps node-fetch, and is only every imported on code that is server-side only. Despite this, I'm getting errors from Vite that node-fetch can't be bundled for the client (which isn't even something I want it doing anyway). Adding node-fetch to optimizeDeps.exclude doesn't help, nor did using import('node-fetch') rather than a static import; I still am getting the error.

I would assume that, between SvelteKit and Vite, the bundler should be smart enough to know that the node-fetch import path is only relevant for the server-side code and not bother trying to prepare it for client-side execution.

SvelteKit itself could definitely be the problem here, but they don't really have any settings related to building; as far as I can tell, it delegates entirely to Vite, which makes me think there's something wrong or missing on the Vite side of things as well.

I think that a concrete improvement that Vite could make here -- with or without SvelteKit being involved -- is being more clear about why the offending package is being imported at all. In my case, the error output looks like this

CleanShot 2022-04-22 at 14 11 48@2x

I see that it's choking on importing promisify from node:utils, and that that is being imported by node-fetch, but it's not clear what module is importing node-fetch; knowing which file of my application is the source of this bundling issue would be really useful in evaluating alternatives that might resolve the problem.

@bluwy bluwy mentioned this issue May 3, 2022
9 tasks
@bluwy bluwy self-assigned this May 20, 2022
@bluwy
Copy link
Member

bluwy commented May 20, 2022

It seems like there's two issues with prebundling here:

  1. Named import of nodejs/builtin modules in dependency fails. This is because Vite's optimizers stubs builtin modules with export default only. I'm trying a few ways locally to fix this.
  2. Dynamic imports should be lazily prebundled. Currently it's eager when analyzing the file for imports. I'm also testing this locally and have a POC that seems to work.

I'll be sending PRs soon.

@bluwy
Copy link
Member

bluwy commented Dec 29, 2022

I found that lazily prebundle for dynamic imports to not always be a good default as some apps may re-prebundle more often which would be poor DX in some cases. I think we could keep it as-is for now given no1 is solved. Closing for now.

@bluwy bluwy closed this as completed Dec 29, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

No branches or pull requests

4 participants