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
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
import { untilUpdated } from '../../testUtils'

test('should load dynamic import with vars', async () => {
await page.click('.foo')
await untilUpdated(() => page.textContent('.view'), 'Foo view')
await page.click('.bar')
await untilUpdated(() => page.textContent('.view'), 'Bar view')
})

test('should load literal dynamic import', async () => {
await page.click('.baz')
await untilUpdated(() => page.textContent('.view'), 'Baz view')
Expand All @@ -16,3 +9,21 @@ test('should load full dynamic import from public', async () => {
await page.click('.qux')
await untilUpdated(() => page.textContent('.view'), 'Qux view')
})

// since this test has a timeout, it should be put last so that it
// does not bleed on the last
test('should load dynamic import with vars', async () => {
await page.click('.foo')
await untilUpdated(() => page.textContent('.view'), 'Foo view')

// first page click will not load the remote message
// because vite needs to compile the lodash dependency
await page.click('.bar')
await untilUpdated(() => page.textContent('.view'), '')

// wait until reload and click again
setTimeout(async () => {
await page.click('.bar')
await untilUpdated(() => page.textContent('.view'), 'Bar view')
}, 10)
})
3 changes: 3 additions & 0 deletions packages/playground/dynamic-import/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,8 @@
"build": "vite build",
"debug": "node --inspect-brk ../../vite/bin/vite",
"serve": "vite preview"
},
"dependencies": {
"lodash": "4.17.21"
}
}
3 changes: 2 additions & 1 deletion packages/playground/dynamic-import/views/bar.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { n } from '../nested/shared'
console.log('bar' + n)
import { isBoolean } from 'lodash'
console.log('bar' + isBoolean(n))

export const msg = 'Bar view'
20 changes: 19 additions & 1 deletion packages/vite/src/node/server/middlewares/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ import {
} from '../../constants'
import { isCSSRequest, isDirectCSSRequest } from '../../plugins/css'

/**
* Time (ms) Vite has to full-reload the page before returning
* an empty response.
*/
const NEW_DEPENDENCY_BUILD_TIMEOUT = 5000
elevatebart marked this conversation as resolved.
Show resolved Hide resolved

const debugCache = createDebugger('vite:cache')
const isDebug = !!process.env.DEBUG

Expand All @@ -48,7 +54,19 @@ export function transformMiddleware(
!req.url?.includes('vite/dist/client')
) {
// missing dep pending reload, hold request until reload happens
server._pendingReload.then(() => res.end())
server._pendingReload.then(() =>
// 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.

// status code request timeout
res.statusCode = 408
res.end(
`<h1>[vite] Something unexpected happened while optimizing "${req.url}"<h1>` +
`<p>The current page should have reloaded by now</p>`
)
}, NEW_DEPENDENCY_BUILD_TIMEOUT)
)
return
}

Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5039,7 +5039,7 @@ lodash@4.17.19:
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.19.tgz#e48ddedbe30b3321783c5b4301fbd353bc1e4a4b"
integrity sha512-JNvd8XER9GQX0v2qJgsaN/mzFCNA5BRe/j8JN9d+tWyGLSodKQHKFicdwNYzWwI3wjRnaKPsGj1XkBjx/F96DQ==

lodash@4.x, lodash@^4.17.15, lodash@^4.17.19, lodash@^4.17.20, lodash@~4.17.15:
lodash@4.17.21, lodash@4.x, lodash@^4.17.15, lodash@^4.17.19, lodash@^4.17.20, lodash@~4.17.15:
version "4.17.21"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.21.tgz#679591c564c3bffaae8454cf0b3df370c3d6911c"
integrity sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==
Expand Down