From: Eduardo San Martin Morote Date: Tue, 1 Sep 2020 15:15:46 +0000 (+0200) Subject: fix(router-view): reuse saved instances in different records (#446) X-Git-Tag: v4.0.0-beta.8~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=65541718b0d5af665fd87dc0e48770cba832a2bb;p=thirdparty%2Fvuejs%2Frouter.git fix(router-view): reuse saved instances in different records (#446) BREAKING CHANGE: `onBeforeRouteLeave` and `onBeforeRouteUpdate` used to have access to the component instance through `instance.proxy` but given that: 1. It has been marked as `internal` (https://github.com/vuejs/vue-next/pull/1849) 2. When using `setup`, all variables are accessible on the scope (and should be accessed that way because the code minimizes better) It has been removed to prevent wrong usage and lighten Vue Router --- diff --git a/__tests__/guards/onBeforeRouteLeave.spec.ts b/__tests__/guards/onBeforeRouteLeave.spec.ts index 17165eb9..139cf5ea 100644 --- a/__tests__/guards/onBeforeRouteLeave.spec.ts +++ b/__tests__/guards/onBeforeRouteLeave.spec.ts @@ -13,58 +13,10 @@ const component = { } describe('onBeforeRouteLeave', () => { - it('invokes with the component context', async () => { - expect.assertions(2) - const spy = jest - .fn() - .mockImplementationOnce(function (this: any, to, from, next) { - expect(typeof this.counter).toBe('number') - next() - }) - const WithLeave = defineComponent({ - template: `text`, - // we use data to check if the context is the right one because saving `this` in a variable logs a few warnings - data: () => ({ counter: 0 }), - setup() { - onBeforeRouteLeave(spy) - }, - }) - - const router = createRouter({ - history: createMemoryHistory(), - routes: [ - { path: '/', component }, - { path: '/leave', component: WithLeave as any }, - ], - }) - const app = createApp({ - template: ` - - `, - }) - app.use(router) - const rootEl = document.createElement('div') - document.body.appendChild(rootEl) - app.mount(rootEl) - - await router.isReady() - await router.push('/leave') - await router.push('/') - expect(spy).toHaveBeenCalledTimes(1) - }) - it('removes guards when leaving the route', async () => { - expect.assertions(3) - const spy = jest - .fn() - .mockImplementation(function (this: any, to, from, next) { - expect(typeof this.counter).toBe('number') - next() - }) + const spy = jest.fn() const WithLeave = defineComponent({ template: `text`, - // we use data to check if the context is the right one because saving `this` in a variable logs a few warnings - data: () => ({ counter: 0 }), setup() { onBeforeRouteLeave(spy) }, diff --git a/__tests__/guards/onBeforeRouteUpdate.spec.ts b/__tests__/guards/onBeforeRouteUpdate.spec.ts index ba6560dc..dab3d99a 100644 --- a/__tests__/guards/onBeforeRouteUpdate.spec.ts +++ b/__tests__/guards/onBeforeRouteUpdate.spec.ts @@ -13,58 +13,10 @@ const component = { } describe('onBeforeRouteUpdate', () => { - it('invokes with the component context', async () => { - expect.assertions(2) - const spy = jest - .fn() - .mockImplementationOnce(function (this: any, to, from, next) { - expect(typeof this.counter).toBe('number') - next() - }) - const WithLeave = defineComponent({ - template: `text`, - // we use data to check if the context is the right one because saving `this` in a variable logs a few warnings - data: () => ({ counter: 0 }), - setup() { - onBeforeRouteUpdate(spy) - }, - }) - - const router = createRouter({ - history: createMemoryHistory(), - routes: [ - { path: '/', component }, - { path: '/foo', component: WithLeave as any }, - ], - }) - const app = createApp({ - template: ` - - `, - }) - app.use(router) - const rootEl = document.createElement('div') - document.body.appendChild(rootEl) - app.mount(rootEl) - - await router.isReady() - await router.push('/foo') - await router.push('/foo?q') - expect(spy).toHaveBeenCalledTimes(1) - }) - it('removes update guards when leaving', async () => { - expect.assertions(3) - const spy = jest - .fn() - .mockImplementation(function (this: any, to, from, next) { - expect(typeof this.counter).toBe('number') - next() - }) + const spy = jest.fn() const WithLeave = defineComponent({ template: `text`, - // we use data to check if the context is the right one because saving `this` in a variable logs a few warnings - data: () => ({ counter: 0 }), setup() { onBeforeRouteUpdate(spy) }, diff --git a/e2e/guards-instances/index.html b/e2e/guards-instances/index.html new file mode 100644 index 00000000..3c257ad6 --- /dev/null +++ b/e2e/guards-instances/index.html @@ -0,0 +1,27 @@ + + + + + + + Vue Router e2e tests - instances in guards + + + + + + << Back to Homepage +
+ +
+ + diff --git a/e2e/guards-instances/index.ts b/e2e/guards-instances/index.ts new file mode 100644 index 00000000..09054518 --- /dev/null +++ b/e2e/guards-instances/index.ts @@ -0,0 +1,206 @@ +import { + createRouter, + createWebHistory, + onBeforeRouteUpdate, + onBeforeRouteLeave, + useRoute, + useRouter, +} from '../../src' +import { createApp, ref, reactive, defineComponent, computed } from 'vue' + +// override existing style on dev with shorter times +if (!__CI__) { + const transitionDuration = '0.5s' + const styleEl = document.createElement('style') + styleEl.innerHTML = ` +.fade-enter-active, +.fade-leave-active { + transition: opacity ${transitionDuration} ease; +} +.child-view { + position: absolute; + transition: all ${transitionDuration} cubic-bezier(0.55, 0, 0.1, 1); +} +` + document.head.append(styleEl) +} + +const Home = defineComponent({ + template: ` +
+

Home

+
+ `, +}) + +const logs = ref([]) + +const state = reactive({ + enter: 0, + update: 0, + leave: 0, +}) + +const Foo = defineComponent({ + template: ` +
+ foo +

{{ enterCallback }}

+

{{ selfUpdates }}

+

{{ selfLeaves }}

+
+ `, + data: () => ({ key: 'Foo', enterCallback: 0, selfUpdates: 0, selfLeaves: 0 }), + // mounted() { + // console.log('mounted Foo') + // }, + beforeRouteEnter(to, from, next) { + state.enter++ + logs.value.push(`enter ${from.path} - ${to.path}`) + next(vm => { + // @ts-ignore + vm.enterCallback++ + }) + }, + beforeRouteUpdate(to, from) { + if (!this || this.key !== 'Foo') throw new Error('no this') + state.update++ + this.selfUpdates++ + logs.value.push(`update ${from.path} - ${to.path}`) + }, + beforeRouteLeave(to, from) { + if (!this || this.key !== 'Foo') throw new Error('no this') + state.leave++ + this.selfLeaves++ + logs.value.push(`leave ${from.path} - ${to.path}`) + }, + + setup() { + onBeforeRouteUpdate((to, from) => { + logs.value.push(`setup:update ${from.path} - ${to.path}`) + }) + onBeforeRouteLeave((to, from) => { + logs.value.push(`setup:leave ${from.path} - ${to.path}`) + }) + return {} + }, +}) + +const webHistory = createWebHistory('/' + __dirname) +const router = createRouter({ + history: webHistory, + routes: [ + { path: '/', component: Home }, + { + path: '/foo', + component: Foo, + }, + { + path: '/f/:id', + component: Foo, + }, + ], +}) + +// preserve existing query +const originalPush = router.push +router.push = to => { + if (typeof to === 'string') { + const resolved = router.resolve(to) + return router.push({ + path: to, + query: { + testCase: router.currentRoute.value.query.testCase, + ...resolved.query, + }, + }) + } else { + return originalPush({ + ...to, + query: { + testCase: router.currentRoute.value.query.testCase, + ...to.query, + }, + }) + } +} + +const app = createApp({ + template: ` +
+

Instances

+

Using {{ testCase || 'default' }}

+ + + + + +
+route: {{ $route.fullPath }}
+enters: {{ state.enter }}
+updates: {{ state.update }}
+leaves: {{ state.leave }}
+      
+
{{ logs.join('\\n') }}
+ +
    +
  • /
  • +
  • /foo
  • +
  • /f/1
  • +
  • /f/2
  • +
  • /f/2?bar=foo
  • +
  • /f/2?foo=key
  • +
  • /f/2?foo=key2
  • +
+ + + + + + + +
+ `, + setup() { + const router = useRouter() + const route = useRoute() + + const testCase = computed({ + get: () => { + let { testCase } = route.query + return !testCase || Array.isArray(testCase) ? '' : testCase + }, + set(testCase) { + router.push({ query: { ...route.query, testCase } }) + }, + }) + + return { state, logs, testCase } + }, +}) + +app.use(router) + +app.mount('#app') diff --git a/e2e/multi-app/index.ts b/e2e/multi-app/index.ts index 65a2ea74..b52ba409 100644 --- a/e2e/multi-app/index.ts +++ b/e2e/multi-app/index.ts @@ -1,4 +1,4 @@ -import { createRouter, createWebHistory, onBeforeRouteUpdate } from '../../src' +import { createRouter, createWebHistory } from '../../src' import { RouteComponent } from '../../src/types' import { createApp, ref, watchEffect, App, inject } from 'vue' @@ -26,15 +26,6 @@ const User: RouteComponent = { setup() { const id = inject('id')! - if (id !== 1) - onBeforeRouteUpdate(function (to, from, next) { - // @ts-ignore - console.log('update from ', id, this.id) - // @ts-ignore - // this.count++ - next() - }) - return { id } }, } diff --git a/e2e/specs/guards-instances.js b/e2e/specs/guards-instances.js new file mode 100644 index 00000000..ae3bcccb --- /dev/null +++ b/e2e/specs/guards-instances.js @@ -0,0 +1,232 @@ +const bsStatus = require('../browserstack-send-status') + +function testCase(browser) { + return browser + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '1') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .click('li:nth-child(3) a') + .assert.containsText('#enterCbs', '2') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '1') + .click('li:nth-child(4) a') + .assert.containsText('#enterCbs', '2') + .assert.containsText('#update', '1') + .assert.containsText('#leave', '1') + .click('li:nth-child(5) a') + .assert.containsText('#enterCbs', '2') + .assert.containsText('#update', '2') + .assert.containsText('#leave', '1') + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '3') + .assert.containsText('#update', '2') + .assert.containsText('#leave', '2') + .expect.element('#logs') + .text.to.equal( + [ + 'enter / - /foo', + 'leave /foo - /f/1', + 'setup:leave /foo - /f/1', + 'enter /foo - /f/1', + 'update /f/1 - /f/2', + 'setup:update /f/1 - /f/2', + 'update /f/2 - /f/2', + 'setup:update /f/2 - /f/2', + 'leave /f/2 - /foo', + 'setup:leave /f/2 - /foo', + 'enter /f/2 - /foo', + ].join('\n') + ) +} + +module.exports = { + ...bsStatus(), + + '@tags': [], + + /** @type {import('nightwatch').NightwatchTest} */ + 'guards instances': function (browser) { + browser + .url('http://localhost:8080/guards-instances/') + .waitForElementPresent('#app > *', 1000) + + .click('#test-normal') + + testCase(browser) + + browser + .click('li:nth-child(1) a') + // the enters are reset when leaving a reused component + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '1') + + browser.end() + }, + + /** @type {import('nightwatch').NightwatchTest} */ + 'guards instances transition': function (browser) { + browser + .url('http://localhost:8080/guards-instances/') + .waitForElementPresent('#app > *', 1000) + + .click('#test-transition') + + testCase(browser) + + browser.end() + }, + + /** @type {import('nightwatch').NightwatchTest} */ + 'guards instances keep alive': function (browser) { + browser + .url('http://localhost:8080/guards-instances/') + .waitForElementPresent('#app > *', 1000) + + .click('#test-keepalive') + + testCase(browser) + + browser + .click('li:nth-child(1) a') + // keep alive keeps the correct instance + .click('li:nth-child(2) a') + .expect.element('#enterCbs') + .text.equals('4') + browser + .click('li:nth-child(1) a') + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '5') + .click('li:nth-child(3) a') + .assert.containsText('#enterCbs', '6') + // leave the update view and enter it again + .click('li:nth-child(1) a') + .click('li:nth-child(3) a') + .click('#resetLogs') + .click('li:nth-child(4) a') + .click('li:nth-child(1) a') + .expect.element('#logs') + .text.to.equal( + [ + 'update /f/1 - /f/2', + 'setup:update /f/1 - /f/2', + 'leave /f/2 - /', + 'setup:leave /f/2 - /', + ].join('\n') + ) + + browser.end() + }, + + /** @type {import('nightwatch').NightwatchTest} */ + 'guards instances keyed': function (browser) { + browser + .url('http://localhost:8080/guards-instances/') + .waitForElementPresent('#app > *', 1000) + + .click('#test-keyed') + + testCase(browser) + + browser + .click('li:nth-child(5) a') + // the query is used as a key resetting the enter count + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '0') + // changing both the route and mounting the component + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '1') + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '1') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '1') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .click('li:nth-child(6) a') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .click('#resetLogs') + .click('li:nth-child(7) a') + .assert.containsText('#enterCbs', '0') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .expect.element('#logs') + .text.to.equal( + ['update /f/2 - /f/2', 'setup:update /f/2 - /f/2'].join('\n') + ) + browser.click('li:nth-child(6) a').assert.containsText('#enterCbs', '0') + + browser.end() + }, + + /** @type {import('nightwatch').NightwatchTest} */ + 'guards instances keepalive keyed': function (browser) { + browser + .url('http://localhost:8080/guards-instances/') + .waitForElementPresent('#app > *', 1000) + + .click('#test-keepalivekeyed') + + testCase(browser) + + browser + .click('li:nth-child(1) a') + // keep alive keeps the correct instance + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '4') + .click('li:nth-child(1) a') + .click('li:nth-child(2) a') + .assert.containsText('#enterCbs', '5') + .click('li:nth-child(3) a') + .assert.containsText('#enterCbs', '6') + + .click('li:nth-child(5) a') + // the query is used as a key resetting the enter count + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '0') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .click('li:nth-child(1) a') + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '1') + .assert.containsText('#update', '0') + .assert.containsText('#leave', '1') + .click('li:nth-child(5) a') + .assert.containsText('#enterCbs', '6') + // on reused instance + .click('li:nth-child(2) a') + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '2') + .assert.containsText('#update', '1') + .assert.containsText('#leave', '1') + .click('#resetLogs') + .click('li:nth-child(7) a') + .assert.containsText('#enterCbs', '0') + // the previous instance was updated but not this one + .assert.containsText('#update', '0') + .assert.containsText('#leave', '0') + .expect.element('#logs') + // should only trigger active guards + .text.to.equal( + ['update /f/2 - /f/2', 'setup:update /f/2 - /f/2'].join('\n') + ) + browser + .click('li:nth-child(6) a') + .assert.containsText('#enterCbs', '2') + .assert.containsText('#update', '2') + .assert.containsText('#leave', '1') + .expect.element('#logs') + .text.to.equal( + [ + 'update /f/2 - /f/2', + 'setup:update /f/2 - /f/2', + 'update /f/2 - /f/2', + 'setup:update /f/2 - /f/2', + ].join('\n') + ) + + browser.end() + }, +} diff --git a/playground/views/ComponentWithData.vue b/playground/views/ComponentWithData.vue index 10a0b399..5c207908 100644 --- a/playground/views/ComponentWithData.vue +++ b/playground/views/ComponentWithData.vue @@ -14,13 +14,14 @@ export default defineComponent({ name: 'ComponentWithData', async setup() { const data = reactive({ other: 'old', fromApi: null }) - data.fromApi = await getData() onBeforeRouteUpdate(async (to, from, next) => { data.fromApi = await getData() next() }) + data.fromApi = await getData() + return { ...toRefs(data), } diff --git a/src/RouterView.ts b/src/RouterView.ts index 5f5dba40..39097640 100644 --- a/src/RouterView.ts +++ b/src/RouterView.ts @@ -11,8 +11,13 @@ import { computed, AllowedComponentProps, ComponentCustomProps, + watch, } from 'vue' -import { RouteLocationNormalized, RouteLocationNormalizedLoaded } from './types' +import { + RouteLocationNormalized, + RouteLocationNormalizedLoaded, + RouteLocationMatched, +} from './types' import { matchedRouteKey, viewDepthKey, @@ -20,6 +25,7 @@ import { } from './injectionSymbols' import { assign } from './utils' import { warn } from './warning' +import { isSameRouteRecord } from './location' export interface RouterViewProps { name?: string @@ -42,7 +48,7 @@ export const RouterViewImpl = defineComponent({ const injectedRoute = inject(routeLocationKey)! const depth = inject(viewDepthKey, 0) - const matchedRouteRef = computed( + const matchedRouteRef = computed( () => (props.route || injectedRoute).matched[depth] ) @@ -51,10 +57,46 @@ export const RouterViewImpl = defineComponent({ const viewRef = ref() + // watch at the same time the component instance, the route record we are + // rendering, and the name + watch( + () => [viewRef.value, matchedRouteRef.value, props.name] as const, + ([instance, to, name], [oldInstance, from, oldName]) => { + // copy reused instances + if (to) { + // this will update the instance for new instances as well as reused + // instances when navigating to a new route + to.instances[name] = instance + // the component instance is reused for a different route or name so + // we copy any saved update or leave guards + if (from && instance === oldInstance) { + to.leaveGuards = from.leaveGuards + to.updateGuards = from.updateGuards + } + } + + // trigger beforeRouteEnter next callbacks + if ( + instance && + to && + // if there is no instance but to and from are the same this might be + // the first visit + (!from || !isSameRouteRecord(to, from) || !oldInstance) + ) { + ;(to.enterCallbacks[name] || []).forEach(callback => + callback(instance) + ) + } + } + ) + return () => { const route = props.route || injectedRoute const matchedRoute = matchedRouteRef.value const ViewComponent = matchedRoute && matchedRoute.components[props.name] + // we need the value at the time we render because when we unmount, we + // navigated to a different location so the value is different + const currentName = props.name if (!ViewComponent) { return slots.default @@ -63,7 +105,7 @@ export const RouterViewImpl = defineComponent({ } // props from route configuration - const routePropsOption = matchedRoute.props[props.name] + const routePropsOption = matchedRoute!.props[props.name] const routeProps = routePropsOption ? routePropsOption === true ? route.params @@ -72,24 +114,16 @@ export const RouterViewImpl = defineComponent({ : routePropsOption : null - // we need the value at the time we render because when we unmount, we - // navigated to a different location so the value is different - const currentName = props.name - const onVnodeMounted = () => { - matchedRoute.instances[currentName] = viewRef.value - ;(matchedRoute.enterCallbacks[currentName] || []).forEach(callback => - callback(viewRef.value!) - ) - } - const onVnodeUnmounted = () => { + const onVnodeUnmounted: VNodeProps['onVnodeUnmounted'] = vnode => { // remove the instance reference to prevent leak - matchedRoute.instances[currentName] = null + if (vnode.component!.isUnmounted) { + matchedRoute!.instances[currentName] = null + } } const component = h( ViewComponent, assign({}, routeProps, attrs, { - onVnodeMounted, onVnodeUnmounted, ref: viewRef, }) diff --git a/src/navigationGuards.ts b/src/navigationGuards.ts index 4093e189..a69929fc 100644 --- a/src/navigationGuards.ts +++ b/src/navigationGuards.ts @@ -17,12 +17,29 @@ import { NavigationFailure, NavigationRedirectError, } from './errors' -import { ComponentOptions } from 'vue' +import { ComponentOptions, onUnmounted, onActivated, onDeactivated } from 'vue' import { inject, getCurrentInstance, warn } from 'vue' import { matchedRouteKey } from './injectionSymbols' import { RouteRecordNormalized } from './matcher/types' import { isESModule } from './utils' +function registerGuard(list: NavigationGuard[], guard: NavigationGuard) { + const removeFromList = () => { + const index = list.indexOf(guard) + if (index > -1) list.splice(index, 1) + } + + onUnmounted(removeFromList) + onDeactivated(removeFromList) + + onActivated(() => { + const index = list.indexOf(guard) + if (index < 0) list.push(guard) + }) + + list.push(guard) +} + /** * Add a navigation guard that triggers whenever the current location is * left. Similarly to {@link beforeRouteLeave}, it has access to the @@ -31,10 +48,8 @@ import { isESModule } from './utils' * @param leaveGuard - {@link NavigationGuard} */ export function onBeforeRouteLeave(leaveGuard: NavigationGuard) { - const instance = getCurrentInstance() - if (!instance) { - __DEV__ && - warn('onBeforeRouteLeave must be called at the top of a setup function') + if (__DEV__ && !getCurrentInstance()) { + warn('onBeforeRouteLeave must be called at the top of a setup function') return } @@ -49,10 +64,7 @@ export function onBeforeRouteLeave(leaveGuard: NavigationGuard) { return } - activeRecord.leaveGuards.push( - // @ts-ignore do we even want to allow that? Passing the context in a composition api hook doesn't make sense - leaveGuard.bind(instance.proxy) - ) + registerGuard(activeRecord.leaveGuards, leaveGuard) } /** @@ -63,10 +75,8 @@ export function onBeforeRouteLeave(leaveGuard: NavigationGuard) { * @param updateGuard - {@link NavigationGuard} */ export function onBeforeRouteUpdate(updateGuard: NavigationGuard) { - const instance = getCurrentInstance() - if (!instance) { - __DEV__ && - warn('onBeforeRouteUpdate must be called at the top of a setup function') + if (__DEV__ && !getCurrentInstance()) { + warn('onBeforeRouteUpdate must be called at the top of a setup function') return } @@ -81,10 +91,7 @@ export function onBeforeRouteUpdate(updateGuard: NavigationGuard) { return } - activeRecord.updateGuards.push( - // @ts-ignore do we even want to allow that? Passing the context in a composition api hook doesn't make sense - updateGuard.bind(instance.proxy) - ) + registerGuard(activeRecord.updateGuards, updateGuard) } export function guardToPromiseFn( diff --git a/src/router.ts b/src/router.ts index b724217c..8575a12d 100644 --- a/src/router.ts +++ b/src/router.ts @@ -709,16 +709,6 @@ export function createRouter(options: RouterOptions): Router { const error = checkCanceledNavigation(toLocation, from) if (error) return error - const [leavingRecords] = extractChangingRecords(toLocation, from) - for (const record of leavingRecords) { - // remove registered guards from removed matched records - record.leaveGuards = [] - record.updateGuards = [] - // free the references - record.instances = {} - record.enterCallbacks = {} - } - // only consider as push if it's not the first navigation const isFirstNavigation = from === START_LOCATION_NORMALIZED const state = !isBrowser ? {} : history.state