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: add a timeout to the res.sep when discovering new dependency #2548

Merged
merged 6 commits into from
Mar 24, 2021
Merged

fix: add a timeout to the res.sep when discovering new dependency #2548

merged 6 commits into from
Mar 24, 2021

Conversation

elevatebart
Copy link
Contributor

@elevatebart elevatebart commented Mar 16, 2021

To avoid returning an erroring file to import in the browser, I choose to wait 5 seconds before returning.
It gives the browser the time it needs to refresh, instead of responding so that request with an invalid empty MIME response.
If the page fails to reload, after 5s the empty invalid response is still sent.

closes #2525

@elevatebart elevatebart marked this pull request as ready for review March 16, 2021 20:56
@elevatebart elevatebart changed the title fix: try adding a timeout to the res.sep fix: add a timeout to the res.sep when discovering new dependency Mar 17, 2021
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@lmiller1990
Copy link

lmiller1990 commented Mar 22, 2021

Anything else you see that needs attention prior to merging this @yyx990803 or @antfu?

// If the refresh has not happened after timeout, Vite considers
// something unexpected has happened. In this case, Vite
// returns an empty response that will error.
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

question: I may be wrong here, but currently the code reads like it always waits statically 5000ms and then respond with a 408.
What makes that it could respond earlier?
Maybe I just misinterpret the issue and just reading the code here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shinigami92 You are absolutely right to ask.
This branch of code is reached when a dependency needs a complete refresh of the page.
Even if the query never responds, the page is going to be refreshed and the query forgotten.

Chronologically:

  • Browser asks for a dependency that is not yet optimized
  • Server looks for it and starts optimization
  • We reach line 57 of this file
  • The server finishes optimizing the new dependency
  • Server sends a WebSocket message to the browser to refresh
  • Optimization process resolves the promise on line 57/58
  • Here is the change in this PR:
    • Before this PR, an empty response was given to the browser before it could refresh. This triggered the error mentioned in the ticket
    • After this PR: If the browser has not refreshed the page after 5 seconds we just drop the request

I hope this chronology clarifies the situation.

@patak-dev patak-dev requested a review from antfu March 22, 2021 19:29
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Mar 23, 2021
@patak-dev patak-dev merged commit 31d10cb into vitejs:main Mar 24, 2021
@elevatebart elevatebart deleted the fix/import-depnedncy-mime-type branch March 24, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Purely dynamic import with dependency returns empty MIME type
6 participants