fix: check the existence of "document" before using it

[mike-lischke]

Description

When loading the client in a worker "document" is not available and should not be used.

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, especially the Pull Request Guidelines.
• 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).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[mike-lischke]

You have large hurdles for a simple pull request, guys. To run prettier I have to install pnpm (which I don't use). I'm not ready to start using it. Can you suggest a different approach without having to do many changes in the repo?

btw. I checked manually that everything in my patch is using the same style as the code around it (2 spaces indentation, no trailing semicolon). What else could be wrong?

[bluwy]

Can you explain how and why the document is being called in workers? Ideally with a repro? Those places where you edit shouldn't get triggered inside workers, but if we did we should probably try to avoid it early on.

[mike-lischke]

This issue is related to #12988 and #9879, so this problem is well known, which makes me scratching my head why this hasn't been fixed months ago. The first linked issue (actually a PR) fixes one place without touching the others.

From what I understood (I have never worked in the vite code base before) the issue stems from the fact that client is imported in each web worker, where document is not available. So the only 2 solutions I can imagine is either to not include client or to protect access to document with a check.

I should add that this issue is related to HMR. Otherwise there's no problem. I have a dozen web workers running in my project and when HMR happens I get a stream of errors about document not being available.

[bluwy]

#12988 was a quick fix as we only then discovered that /@vite/client snucked into workers code. It shouldn't have been loaded in the first place. #14918 was a fix that tried to prevent that.

I assume in your workers code you imported things that had HMR, and that indirectly brought it /@vite/client? I guess I could see that happen. I'm currently inclined to disable HMR entirely for workers for now by skipping this part:

vite/packages/vite/src/client/client.ts

Lines 19 to 71 in 3e42611

	console.debug('[vite] connecting...')
	const importMetaUrl = new URL(import.meta.url)
	// use server configuration, then fallback to inference
	const serverHost = __SERVER_HOST__
	const socketProtocol =
	__HMR_PROTOCOL__ || (importMetaUrl.protocol === 'https:' ? 'wss' : 'ws')
	const hmrPort = __HMR_PORT__
	const socketHost = `${__HMR_HOSTNAME__ || importMetaUrl.hostname}:${
	hmrPort || importMetaUrl.port
	}${__HMR_BASE__}`
	const directSocketHost = __HMR_DIRECT_TARGET__
	const base = __BASE__ || '/'
	const messageBuffer: string[] = []
	let socket: WebSocket
	try {
	let fallback: (() => void) | undefined
	// only use fallback when port is inferred to prevent confusion
	if (!hmrPort) {
	fallback = () => {
	// fallback to connecting directly to the hmr server
	// for servers which does not support proxying websocket
	socket = setupWebSocket(socketProtocol, directSocketHost, () => {
	const currentScriptHostURL = new URL(import.meta.url)
	const currentScriptHost =
	currentScriptHostURL.host +
	currentScriptHostURL.pathname.replace(/@vite\/client$/, '')
	console.error(
	'[vite] failed to connect to websocket.\n' +
	'your current setup:\n' +
	` (browser) ${currentScriptHost} <--[HTTP]--> ${serverHost} (server)\n` +
	` (browser) ${socketHost} <--[WebSocket (failing)]--> ${directSocketHost} (server)\n` +
	'Check out your Vite / network configuration and https://vitejs.dev/config/server-options.html#server-hmr .',
	)
	})
	socket.addEventListener(
	'open',
	() => {
	console.info(
	'[vite] Direct websocket connection fallback. Check out https://vitejs.dev/config/server-options.html#server-hmr to remove the previous connection error.',
	)
	},
	{ once: true },
	)
	}
	}
	socket = setupWebSocket(socketProtocol, socketHost, fallback)
	} catch (error) {
	console.error(`[vite] failed to connect to websocket (${error}). `)
	}

But I might need some thoughts from other maintainers about this. With this PR's changes instead, maybe we can get HMR to work in workers, but somewhat only partially and that could be nice to have.

[mike-lischke]

I see the errors also when applying changes to code which is definitely not used in web workers (e.g. preact stuff).

I wouldn't change the entire worker handling in vite. It's working well so far. Only these errors are a nuisance. And checking something that might not be available before using it is always a good thing.

[hi-ogawa]

I assume in your workers code you imported things that had HMR, and that indirectly brought it /@vite/client? I guess I could see that happen.

I investigated a similar issue in the past #15036 (comment), so I just wanted to chime in. Sorry if it's a different a topic.

I think there's a one pitfall when using Vite Preact plugin (compared to React plugin), which is that Preact plugin applies hmr-related transform on plain js/ts files by default, but on the other hand, react/babel plugin only applies on jsx/tsx files. (Actually there seems to be one check to skip id.includes('?worker') but this works only for worker entry file. https://github.com/preactjs/prefresh/blob/51f8c7fec4abaf52a2e13f15b90f90544a5cabb9/packages/vite/src/index.js#L24)

In addition to this, Preact seems to apply transform to class component (e.g. class SomeNormalClass {}) as well so it's more prone to make false-positive and inject import.meta.hot related code in worker (and thus Vite finally injecting /@vite/client).

So, the difference of the reproduction condition is:

• with react plugin, function SomeCaptionFn() {} in some-worker-dep.tsx.
• with preact plugin, class SomeClass() {} in some-worker-dep.ts.

I created a quick repro here:

• preact https://stackblitz.com/edit/github-qxc1xw?file=src%2Fworker-repro-dep.ts
• react https://stackblitz.com/edit/github-g5epcg?file=src%2Fworker-repro.ts

I checked plugin's transform from a devtool network tab. For example, you can see /src/worker-repro-dep.ts which just has class CapitalClass ended up with import.meta.hot related code.

part of transformed code

export class CapitalClass {
    something() {}
}
_c = CapitalClass;
var _c;
$RefreshReg$(_c, "CapitalClass");

if (import.meta.hot) {
    self.$RefreshReg$ = prevRefreshReg;
    self.$RefreshSig$ = prevRefreshSig;
    import.meta.hot.accept((m)=>{
        try {
            flushUpdates();
        } catch (e) {
            self.location.reload();
        }
    }
    );
}

One quick workaround for Preact users could be to explicitly set a plugin option like preact({ include: /\.(j|t)sx$/ }) so that js/ts won't be affected by transform (or setup exclude: ... to exclude worker related code from preact plugin).

---

I'm currently inclined to disable HMR entirely for workers for now by skipping this part

I think that might also help Audioworklet users who somehow unintentionally ends up with /@vite/client, which is the issue came up in #15036. Even though I totally understand Audioworklet is super niche use case and it might not worth supporting officially, but it would be nice if it works without workaround on user side.

[ArnaudBarre]

I can confirm @hi-ogawa feedback. I saw multiple people having issues with that by having React component imported in a worker, which was often not wanted and caused by barrel exports. Having .tsx import feel strange in that case. I don't know how HMR works in Preact, but if HMR is the same with preact({ include: /\.(j|t)sx$/ }), I think this should be used for now and maybe upstream the 'issue' if this was added just by the old bad habit of using JSX inside JS.

I prefer to not add to many edge case for this so that if someone find a way to get HMR working in worker we don't have to deal with people having currently half-working injected client

[mike-lischke]

@ArnaudBarre what is a "barrel export"? Never heard this term before.

And what do you actually mean by "I prefer to not add many edge case for this"? Are you referring to fixes for this problem? Or considering cases like this one (which doesn't seem to be an edge case, from what I read so far).

Do you prefer not to fix this issue and annoy people until sometimes in the future someone will get something else to work? And may I remind you, this PR is not about preact code! I especially took care not to include any heavy code except what the worker is specifically for (text parsing).

[ArnaudBarre]

See https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/ for a look at this and their various issue

What I mean is that don't like adding more code that will result in no-op code running in the worker and that would make it harder to deal with latter on. The code is clean, it's just fixing the issue at the last step instead of either:

• avoid HMR code in workers
• add HMR in workers

[mike-lischke]

Thanks for the link. An interesting read, for sure!

I understand your concern about adding code now that may be obsolete later. But what are the other options? Live with the current behavior until the HMR code is changed to avoid this situation? As we all know it's difficult to predict a timeframe for something to happen, especially in projects which are run by volunteers. So it might take some time until this change to happen. All a matter of priorities, of course. My opinion: Grab the low hanging fruit now and remove the checks, once the HMR issue is resolved.

[ArnaudBarre]

and remove the checks, once the HMR issue is resolved.

This could be a breaking change in the future because people would have start to have code that rely on the fact that HMR code in a worker is no-op and it's impossible to be sure how adding it will affect usages

This is a tradeoff, so we need a good reason to take the risk of migrating back, and I don't see a clear example where the issue cannot be solved by having a cleaner split between worker code and UI code (which often will lead to smaller worker bundle which is good)

[sapphi-red]

Closing as this change seems to be covered by #16318