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(vue-app): skip page render early on error or navigation #20719

Merged
merged 5 commits into from May 22, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 50 additions & 9 deletions packages/vue-app/template/client.js
Expand Up @@ -244,7 +244,7 @@ function resolveComponents (route) {
<% } %>

<% if (features.middleware) { %>
function callMiddleware (Components, context, layout) {
function callMiddleware (Components, context, layout, renderState) {
let midd = <%= devalue(router.middleware) %><%= isTest ? '// eslint-disable-line' : '' %>
let unknownMiddleware = false

Expand Down Expand Up @@ -278,7 +278,7 @@ function callMiddleware (Components, context, layout) {
if (unknownMiddleware) {
return
}
return middlewareSeries(midd, context)
return middlewareSeries(midd, context, renderState)
}
<% } else if (isDev) {
// This is a placeholder function mainly so we dont have to
Expand All @@ -288,7 +288,7 @@ function callMiddleware () {
return Promise.resolve(true)
}
<% } %>
async function render (to, from, next) {
async function render (to, from, next, renderState) {
if (this._routeChanged === false && this._paramChanged === false && this._queryChanged === false) {
return next()
}
Expand Down Expand Up @@ -329,6 +329,12 @@ async function render (to, from, next) {
await setContext(app, {
route: to,
from,
error: (err) => {
if (renderState.aborted) {
return
}
app.nuxt.error.call(this, err)
},
next: _next.bind(this)
})
this._dateLastError = app.nuxt.dateErr
Expand All @@ -342,10 +348,14 @@ async function render (to, from, next) {
if (!Components.length) {
<% if (features.middleware) { %>
// Default layout
await callMiddleware.call(this, Components, app.context)
await callMiddleware.call(this, Components, app.context, undefined, renderState)
if (nextCalled) {
return
}
if (renderState.aborted) {
next(false)
return
}
<% } %>

<% if (features.layouts) { %>
Expand All @@ -359,10 +369,14 @@ async function render (to, from, next) {
<% } %>

<% if (features.middleware) { %>
await callMiddleware.call(this, Components, app.context, layout)
await callMiddleware.call(this, Components, app.context, layout, renderState)
if (nextCalled) {
return
}
if (renderState.aborted) {
next(false)
return
}
<% } %>

// Show error page
Expand All @@ -387,10 +401,14 @@ async function render (to, from, next) {
try {
<% if (features.middleware) { %>
// Call middleware
await callMiddleware.call(this, Components, app.context)
await callMiddleware.call(this, Components, app.context, undefined, renderState)
if (nextCalled) {
return
}
if (renderState.aborted) {
next(false)
return
}
if (app.context._errored) {
return next()
}
Expand All @@ -407,10 +425,14 @@ async function render (to, from, next) {

<% if (features.middleware) { %>
// Call middleware for layout
await callMiddleware.call(this, Components, app.context, layout)
await callMiddleware.call(this, Components, app.context, layout, renderState)
if (nextCalled) {
return
}
if (renderState.aborted) {
next(false)
return
}
if (app.context._errored) {
return next()
}
Expand Down Expand Up @@ -578,10 +600,18 @@ async function render (to, from, next) {
this.$loading.finish()
}
<% } %>
if (renderState.aborted) {
next(false)
return
}
next()
}

} catch (err) {
if (renderState.aborted) {
next(false)
return
}
const error = err || {}
if (error.message === 'ERR_REDIRECT') {
return this.<%= globals.nuxt %>.$emit('routeChanged', to, from, error)
Expand Down Expand Up @@ -908,7 +938,17 @@ async function mountApp (__app) {

// Add beforeEach router hooks
router.beforeEach(loadAsyncComponents.bind(_app))
router.beforeEach(render.bind(_app))

// Each new invocation of render() aborts previous invocation
let renderState = null
const boundRender = render.bind(_app)
router.beforeEach((to, from, next) => {
if (renderState) {
renderState.aborted = true
}
renderState = { aborted: false }
boundRender(to, from, next, renderState)
})

// Fix in static: remove trailing slash to force hydration
// Full static, if server-rendered: hydrate, to allow custom redirect to generated page
Expand Down Expand Up @@ -954,5 +994,6 @@ async function mountApp (__app) {
errorHandler(err)
}
})
})
},
{ aborted: false })
Copy link
Author

Choose a reason for hiding this comment

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

I have not put too much thought into that part of the code (the SPA case). I could look more if needed but it could be just left like that, without supporting early abort.

}
10 changes: 7 additions & 3 deletions packages/vue-app/template/utils.js
Expand Up @@ -279,6 +279,10 @@ export async function setContext (app, context) {
app.context.from = fromRouteData
}

if (context.error) {
app.context.error = context.error
}

app.context.next = context.next
app.context._redirected = false
app.context._errored = false
Expand All @@ -287,13 +291,13 @@ export async function setContext (app, context) {
app.context.query = app.context.route.query || {}
}
<% if (features.middleware) { %>
export function middlewareSeries (promises, appContext) {
if (!promises.length || appContext._redirected || appContext._errored) {
export function middlewareSeries (promises, appContext, renderState) {
if (!promises.length || appContext._redirected || appContext._errored || (renderState && renderState.aborted)) {
return Promise.resolve()
}
return promisify(promises[0], appContext)
.then(() => {
return middlewareSeries(promises.slice(1), appContext)
return middlewareSeries(promises.slice(1), appContext, renderState)
})
}
<% } %>
Expand Down
68 changes: 68 additions & 0 deletions test/e2e/basic.navigation.test.js
@@ -0,0 +1,68 @@
import Browser from '../utils/browser'
import { loadFixture, getPort, Nuxt, waitFor } from '../utils'

let port
const browser = new Browser()
const url = route => 'http://localhost:' + port + route

let nuxt = null
let page = null

describe('basic browser navigation', () => {
beforeAll(async () => {
const config = await loadFixture('basic')
nuxt = new Nuxt(config)
await nuxt.ready()

port = await getPort()
await nuxt.server.listen(port, 'localhost')

await browser.start({
// slowMo: 50,
// headless: false
})
})

const TEST_PATHS = [
'asyncData-error',
'asyncData-throw',
'middleware-asyncData-error',
'middleware-asyncData-throw'
]

for (const path of TEST_PATHS) {
test(`Navigate away from /navigation/${path}`, async () => {
page = await browser.page(url('/'))
expect(await page.$text('h1')).toBe('Index page')

await page.nuxt.navigate('/navigation')
expect(await page.$text('h3')).toContain('Click one of the links below')

await page.nuxt.navigate(`/navigation/${path}`, /* waitEnd */false)
await waitFor(nuxt.options.loading.throttle + 100)
const loading = await page.nuxt.loadingData()
expect(loading.show).toBe(true)

await page.nuxt.go(-1)
await page.waitForFunction(
'$nuxt.$loading.$data.show === false'
)
expect(await page.$text('h1')).toBe('Index page')

await waitFor(2000)

expect(await page.$text('h1')).toBe('Index page')
})
}

// Close server and ask nuxt to stop listening to file changes
afterAll(async () => {
await nuxt.close()
})

// Stop browser
afterAll(async () => {
await page.close()
await browser.close()
})
})
3 changes: 3 additions & 0 deletions test/fixtures/basic/middleware/slow.js
@@ -0,0 +1,3 @@
export default async function () {
await new Promise(resolve => setTimeout(resolve, 2000))
}
3 changes: 3 additions & 0 deletions test/fixtures/basic/pages/index.vue
Expand Up @@ -2,5 +2,8 @@
<div>
<h1>Index page</h1>
<img src="~/assets/test.svg">
<nuxt-link to="navigation">
Continue to /navigation
</nuxt-link>
</div>
</template>
27 changes: 27 additions & 0 deletions test/fixtures/basic/pages/navigation.vue
@@ -0,0 +1,27 @@
<template>
<div>
<h3>Click one of the links below and navigate back in history before the page has fully loaded. Pass if no error page shown.</h3>
<ul>
<li v-for="test in tests" :key="test">
<nuxt-link :to="'navigation/' + test">
{{ test }}
</nuxt-link>
</li>
</ul>
</div>
</template>

<script>
export default {
computed: {
tests () {
return [
'asyncData-error',
'asyncData-throw',
'middleware-asyncData-error',
'middleware-asyncData-throw'
]
}
}
}
</script>
11 changes: 11 additions & 0 deletions test/fixtures/basic/pages/navigation/asyncData-error.vue
@@ -0,0 +1,11 @@
<template>
<h1>Async Data Error</h1>
</template>
<script>
export default {
async asyncData ({ error }) {
await new Promise(resolve => setTimeout(resolve, 2000))
error({ statusCode: 401, message: 'Not allowed here' })
}
}
</script>
11 changes: 11 additions & 0 deletions test/fixtures/basic/pages/navigation/asyncData-throw.vue
@@ -0,0 +1,11 @@
<template>
<h1>Async Throw Error</h1>
</template>
<script>
export default {
async asyncData () {
await new Promise(resolve => setTimeout(resolve, 2000))
throw new Error('crash')
}
}
</script>
@@ -0,0 +1,11 @@
<template>
<h1>Async Data Error</h1>
</template>
<script>
export default {
middleware: ['slow'],
asyncData ({ error, route }) {
error({ statusCode: 401, message: 'Not allowed here' })
}
}
</script>
@@ -0,0 +1,11 @@
<template>
<h1>Async Throw Error</h1>
</template>
<script>
export default {
middleware: ['slow'],
asyncData () {
throw new Error('crash')
}
}
</script>