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: undefined document in worker #12988

Merged
merged 2 commits into from Apr 26, 2023
Merged

fix: undefined document in worker #12988

merged 2 commits into from Apr 26, 2023

Conversation

patak-dev
Copy link
Member

Fixes #12970

Description

Regression added by:


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Apr 24, 2023

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

@patak-dev
Copy link
Member Author

patak-dev commented Apr 25, 2023

Bringing some notes we are discussing with @ArnaudBarre. Why this hasn't been an issue so far? I tested the repro at #12970, and if you do HMR you will get an error when checking for hasErrorOverlay:

client.ts:288 Uncaught (in promise) ReferenceError: document is not defined
    at hasErrorOverlay (client.ts:288:3)
    at handleMessage (client.ts:150:28)
    at WebSocket.<anonymous> (client.ts:91:5)

I tested with vite@4.0.0 and the issue is still there, so it isn't like we just started including the client in workers in the last minor. Folks may just cope with this error?
If you modify the main thread source code, it will HMR, the error appears but everything still works. If you modify the workers code, then it full-reloads so there is no error. In both cases the page is in a proper state, just with a noisy error in one case. @subframe7536 is this what you were experiencing before 4.3, no?

So we don't have HMR in workers AFAICS, we will full reload.

This PR isn't really fixing the real issue, but at least it patches things so previous apps can at least work. I think we should merge it as a temporal patch and maybe we should guard every document access in the worker.

@subframe7536
Copy link

yes, and this pr makes all work

@patak-dev
Copy link
Member Author

Let's merge this one as a temporal patch. See more context at #12995

@patak-dev patak-dev merged commit 08c1452 into main Apr 26, 2023
20 checks passed
@patak-dev patak-dev deleted the fix/document-use-in-workers branch April 26, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document is not defined when using xxxx.wasm?url or ?worker in vite 4.3.1
3 participants