fix: use Bun's implementation of `ws` instead of the bundled one #13901

paperdave · 2023-07-20T02:46:37Z

Description

There are two small issues that are blocking Vite from working in Bun, which is much easier to fix in the Vite project itself.

Bun implements it's own version of ws, which gets overridden when doing import {...} from "ws". Vite bundles it's own copy of ws which attempts to use req.socket.write which we do not implement as we do not use a node stream for the underlying network sockets. Implementing the APIs needed for the JS implementation of ws would take too long and be much slower.
During config loading there is a directory caching bug, the workaround we are using is applying a query string parameter to the file:// url used when loading the config through esm.

That is all that it takes to get Vite working on Bun, though it depends on a handful of various fixes we recently made to our node compatibility. To test these changes, you need to use the canary version bun upgrade --canary (or 0.7 when released), and then use bun --bun run dev (--bun forces bun's runtime over node's).

Closes oven-sh/bun#250

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

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

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the PR Title Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

bluwy · 2023-07-20T06:07:11Z

packages/vite/src/node/config.ts

+    // Bun will cache file/directories, and since we just created the file it might throw a ResolveError
+    // Adding a query string with the `?` will bust the cache.
+    // See https://github.com/oven-sh/bun/issues/3216
+    const fileUrl = `${pathToFileURL(fileBase)}.mjs?`


Not oppose to this change, but I feel like Bun should ultimately fix this too. Currently the issue is closed and appending ? is the way in Bun?

Agreed, we could move forward with this one but it will be good to remove this hack as soon as Bun works out this bug. I think we should only append ? when process.versions.bun.

Bun's builtin TypeScript, JSX, ESM, and CommonJS support means transpiling a configuration file ahead of time should be unnecessary. Reading, transpiling, saving and then reading the configuration file again makes Vite start slower than it could. Rather than conditionally adding a ?, what if we plan to disable this step entirely in Bun?

That would lose slight parity with the current config loader, as it tracks the dependencies of the config to trigger a server restart. Personally, I think this "feature" has blocked us from simplifying a lot of things that I'd rather not have it (around Vite 5? Not sure yet).

So I guess we have two options (that I'm fine either ways):

Use ? cache bust, but Bun should fix this eventually.

Load config with Bun without deps restart trigger. Bun users have slightly different experience.

I lean toward using ? here if we can't get the same experience.

@Jarred-Sumner replying to your comment (#13901 (comment)), I think it would be best if we can implement the least changes on our end here to support Bun. If no2 requires using more Bun-specific APIs, that would likely create another code path to maintain.

Is oven-sh/bun#3216 (the ? issue) not fixable on Bun's end? I feel like this quirk could affect more libraries in general than only Vite.

The reason we put the ? is because the fix for bun isn't trivial. I agree we should fix this in bun, but it might not happen today for 0.7.

patak-dev · 2023-07-20T07:39:13Z

@paperdave thanks for the PR. Would you separate the directory caching bug part on a separate PR so we can better track both changes? (and revert this one later if we go forward with it). We would like to know for this part what are your plans, as it seems we are patching a bug that may be solved soon in Bun.

This PR could remain about Bun's own version of ws, which I think we can merge without much discussion.

packages/vite/src/node/server/ws.ts

Jarred-Sumner · 2023-07-20T13:10:02Z

Load config with Bun without deps restart trigger. Bun users have slightly different experience.

In Bun, you can do Loader.getDependencyKeysIfEvaluated and it will call this function which returns an array of module specifiers a module depends on https://github.com/WebKit/WebKit/blob/2f688c542e6be822158997052c6fe6f6f63c4c16/Source/JavaScriptCore/builtins/ModuleLoader.js#L593 I don’t feel good we expose this JSC internal api so directly because they may change it randomly later, but we can expose a wrapper for this Another thing in Bun is that require.cache can be used to invalidate ESM modules. require.cache is implemented as a Proxy which merges the real CommonJS module cache and the ESM registry cache into one

On Thu, Jul 20, 2023 at 6:02 AM Bjorn Lu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/vite/src/node/config.ts <#13901 (comment)>: > + // Bun will cache file/directories, and since we just created the file it might throw a ResolveError + // Adding a query string with the `?` will bust the cache. + // See oven-sh/bun#3216 + const fileUrl = `${pathToFileURL(fileBase)}.mjs?` That would lose slight parity with the current config loader, as it tracks the dependencies of the config to trigger a server restart. Personally, I think this "feature" has blocked us from simplifying a lot of things that I'd rather not have it (around Vite 5? Not sure yet). So I guess we have two options: 1. Use ? cache bust, but Bun should fix this eventually. 2. Load config with Bun without deps restart trigger. Bun users have slightly different experience. — Reply to this email directly, view it on GitHub <#13901 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFNGS4MW66HCNIZNBTEYVLXRET5NANCNFSM6AAAAAA2QYJQQQ> . You are receiving this because you commented.Message ID: ***@***.***>

(Meant as a reply to a review comment but shows up as a top level comment because email )

paperdave · 2023-07-20T21:36:09Z

thanks for the PR. Would you separate the directory caching bug part on a separate PR so we can better track both changes? (and revert this one later if we go forward with it). We would like to know for this part what are your plans, as it seems we are patching a bug that may be solved soon in Bun.

This PR could remain about Bun's own version of ws, which I think we can merge without much discussion.

I think that is reasonable. I'll split this into two PRs.

paperdave · 2023-07-20T22:15:22Z

the ? thing has been fixed in bun with a less-than-ideal solution but it will work for this. this should also mean older versions of vite will get past config loading, but only break on hmr.

patak-dev · 2023-07-21T08:06:46Z

Thanks for moving the ? workaround to Bun's side @paperdave. We released this PR as part of vite@4.4.6

paperdave · 2023-07-21T08:08:00Z

legendary. were gonna release bun 0.7 tomorrow (i think) so that will be when users can test it out.

Jarred-Sumner mentioned this pull request Jul 20, 2023

Vite oven-sh/bun#250

Closed

bluwy changed the title ~~Make vite work in Bun~~ feat: make vite work in Bun Jul 20, 2023

bluwy reviewed Jul 20, 2023

View reviewed changes

ArnaudBarre reviewed Jul 20, 2023

View reviewed changes

packages/vite/src/node/server/ws.ts Show resolved Hide resolved

Use bun's ws module for vite

ff90b01

paperdave changed the title ~~feat: make vite work in Bun~~ fix: use Bun's implementation of ws instead of the bundled one Jul 20, 2023

patak-dev approved these changes Jul 21, 2023

View reviewed changes

bluwy approved these changes Jul 21, 2023

View reviewed changes

patak-dev merged commit 049404c into vitejs:main Jul 21, 2023
14 checks passed

This was referenced Jul 21, 2023

Bump vite to 4.4.6 withastro/astro#7746

Merged

Support Astro oven-sh/bun#3746

Open

antfu mentioned this pull request Jul 30, 2023

feat: add bun support antfu/unconfig#24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use Bun's implementation of `ws` instead of the bundled one #13901

fix: use Bun's implementation of `ws` instead of the bundled one #13901

paperdave commented Jul 20, 2023

bluwy Jul 20, 2023

patak-dev Jul 20, 2023

Jarred-Sumner Jul 20, 2023

bluwy Jul 20, 2023 •

edited

patak-dev Jul 20, 2023

bluwy Jul 20, 2023

paperdave Jul 20, 2023

patak-dev commented Jul 20, 2023

Jarred-Sumner commented Jul 20, 2023 via email •

edited

paperdave commented Jul 20, 2023

paperdave commented Jul 20, 2023

patak-dev commented Jul 21, 2023

paperdave commented Jul 21, 2023

fix: use Bun's implementation of ws instead of the bundled one #13901

fix: use Bun's implementation of ws instead of the bundled one #13901

Conversation

paperdave commented Jul 20, 2023

Description

Additional context

What is the purpose of this pull request?

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

bluwy Jul 20, 2023

Choose a reason for hiding this comment

patak-dev Jul 20, 2023

Choose a reason for hiding this comment

Jarred-Sumner Jul 20, 2023

Choose a reason for hiding this comment

bluwy Jul 20, 2023 • edited

Choose a reason for hiding this comment

patak-dev Jul 20, 2023

Choose a reason for hiding this comment

bluwy Jul 20, 2023

Choose a reason for hiding this comment

paperdave Jul 20, 2023

Choose a reason for hiding this comment

patak-dev commented Jul 20, 2023

Jarred-Sumner commented Jul 20, 2023 via email • edited

paperdave commented Jul 20, 2023

paperdave commented Jul 20, 2023

patak-dev commented Jul 21, 2023

paperdave commented Jul 21, 2023

fix: use Bun's implementation of `ws` instead of the bundled one #13901

fix: use Bun's implementation of `ws` instead of the bundled one #13901

bluwy Jul 20, 2023 •

edited

Jarred-Sumner commented Jul 20, 2023 via email •

edited