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

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

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

Conversation

schontz
Copy link

@schontz schontz commented Sep 26, 2024

Description

Fixes #17570

Copy link

stackblitz bot commented Sep 26, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@schontz schontz changed the title Only run DOM operations in the client when there is a document (#17570) fix: Only run DOM operations in the client when there is a document (#17570) Sep 26, 2024
@schontz schontz changed the title fix: Only run DOM operations in the client when there is a document (#17570) fix: only run DOM operations in the client when there is a document (#17570) Sep 26, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

I think we need a warning that tells importing CSS inside worker does not work.

Comment on lines +211 to +214
if (!hasDocument) {
return
}

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.

`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.

@sapphi-red sapphi-red added feat: css p2-nice-to-have Not breaking anything but nice to have (priority) feat: web workers labels Sep 30, 2024
@schontz
Copy link
Author

schontz commented Sep 30, 2024

I ran prettier and created another commit but the linter still fails. Not sure what to do.

@sapphi-red
Copy link
Member

I ran prettier and created another commit but the linter still fails. Not sure what to do.

Probably you need to run pnpm i before running pnpm format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css feat: web workers p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document not defined in worker import
2 participants