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.

Sign up for GitHub

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

Jump to bottom

fix: only run DOM operations in the client when there is a document (#17570) #18202

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: only run DOM operations in the client when there is a document (#17570) #18202

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9631db1
Only run DOM operations in the client when there is a document
schontz Sep 26, 2024
2e783c8
Merge pull request #1 from schontz/schontz-patch-17570
schontz Sep 26, 2024
d014142
Fix prettier
schontz Sep 26, 2024
00145ca
Merge branch 'vitejs:main' into main
schontz Sep 26, 2024
eb81a28
Prettier (again)
schontz Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions packages/vite/src/client/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ async function handleMessage(payload: HotPayload) {
return hmrClient.queueUpdate(update)
}

if (!hasDocument) {
return
}

Comment on lines +211 to +214
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only change needed is this line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we only change this line? There are many DOM operations that will fail in workers, so it seemed best to make each one of them safe. Even if some may be safe today (e.g., first hasDocument check prevents others from running), if the code paths change at all they will throw errors as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's true. I actually forgot about this line.

vite/packages/vite/src/node/plugins/css.ts

Line 522 in a577828

`import { updateStyle as __vite__updateStyle, removeStyle as __vite__removeStyle } from ${JSON.stringify(

While I think adding hasDocument check everywhere feels error prone as well, I think it's fine for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is still somewhat error prone, and there are other latent bugs, such as location.reload(), which isn't available in workers either.

I suppose we could make the client more headless with hooks and guard all the "has DOM" things in one spot, but that would require some extra refactoring.

// css-update
// this is only sent when a css file referenced with <link> is updated
const { path, timestamp } = update
Expand Down Expand Up @@ -314,16 +318,20 @@ const enableOverlay = __HMR_ENABLE_OVERLAY__
const hasDocument = 'document' in globalThis

function createErrorOverlay(err: ErrorPayload['err']) {
clearErrorOverlay()
document.body.appendChild(new ErrorOverlay(err))
if (hasDocument) {
clearErrorOverlay()
document.body.appendChild(new ErrorOverlay(err))
}
}

function clearErrorOverlay() {
document.querySelectorAll<ErrorOverlay>(overlayId).forEach((n) => n.close())
if (hasDocument) {
document.querySelectorAll<ErrorOverlay>(overlayId).forEach((n) => n.close())
}
}

function hasErrorOverlay() {
return document.querySelectorAll(overlayId).length
return hasDocument ? document.querySelectorAll(overlayId).length : 0
}

async function waitForSuccessfulPing(
Expand Down Expand Up @@ -357,7 +365,7 @@ async function waitForSuccessfulPing(
await wait(ms)

while (true) {
if (document.visibilityState === 'visible') {
if (hasDocument && document.visibilityState === 'visible') {
if (await ping()) {
break
}
Expand All @@ -374,6 +382,9 @@ function wait(ms: number) {

function waitForWindowShow() {
return new Promise<void>((resolve) => {
if (!hasDocument) {
return
}
const onChange = async () => {
if (document.visibilityState === 'visible') {
resolve()
Expand All @@ -388,24 +399,27 @@ const sheetsMap = new Map<string, HTMLStyleElement>()

// collect existing style elements that may have been inserted during SSR
// to avoid FOUC or duplicate styles
if ('document' in globalThis) {
if (hasDocument) {
document
.querySelectorAll<HTMLStyleElement>('style[data-vite-dev-id]')
.forEach((el) => {
sheetsMap.set(el.getAttribute('data-vite-dev-id')!, el)
})
}

const cspNonce =
'document' in globalThis
? document.querySelector<HTMLMetaElement>('meta[property=csp-nonce]')?.nonce
: undefined
const cspNonce = hasDocument
? document.querySelector<HTMLMetaElement>('meta[property=csp-nonce]')?.nonce
: undefined

// all css imports should be inserted at the same position
// because after build it will be a single css file
let lastInsertedStyle: HTMLStyleElement | undefined

export function updateStyle(id: string, content: string): void {
if (!hasDocument) {
return
}

let style = sheetsMap.get(id)
if (!style) {
style = document.createElement('style')
Expand Down Expand Up @@ -435,6 +449,9 @@ export function updateStyle(id: string, content: string): void {
}

export function removeStyle(id: string): void {
if (!hasDocument) {
return
}
const style = sheetsMap.get(id)
if (style) {
document.head.removeChild(style)
Expand Down