Fixed routerview state rollback on redirects and guarded delayed focu…

src/focus/focus.js

@@ -41,6 +41,8 @@ export default {
     return focusedComponent
   },
   set(component, event) {
+    if (component === undefined || component.eol === true) return
+
     clearTimeout(setFocusTimeout)
 
     // early return if already focused
@@ -75,7 +77,7 @@ export default {
 
     // and finally set focus to the leaf component
     setFocusTimeout = setTimeout(
-      () => setFocus(component, event),
+      () => component.eol !== true && setFocus(component, event),
       this.hold === true ? Settings.get('holdTimeout', DEFAULT_HOLD_TIMEOUT_MS) : 0
     )
   },

src/focus/focus.test.js

@@ -214,6 +214,48 @@ test('Unfocus partial focus chain', (assert) => {
   })
 })
 
+test('Ignore focus requests for destroyed components', (assert) => {
+  const focusedComponent = Focus.get()
+  const destroyedComponent = {
+    eol: true,
+    [symbols.lifecycle]: {
+      state: 'destroy',
+    },
+  }
+
+  Focus.set(destroyedComponent)
+
+  assert.equal(Focus.get(), focusedComponent, 'Destroyed component should not receive focus')
+  assert.end()
+})
+
+test('Ignore delayed focus callback when component is destroyed', (assert) => {
+  const focusedComponent = Focus.get()
+  const component = {
+    eol: false,
+    [symbols.lifecycle]: {
+      state: 'init',
+    },
+  }
+
+  Focus.set(component)
+  component.eol = true
+
+  setTimeout(() => {
+    assert.equal(
+      Focus.get(),
+      focusedComponent,
+      'Destroyed component should not receive delayed focus'
+    )
+    assert.equal(
+      component[symbols.lifecycle].state,
+      'init',
+      'Destroyed component lifecycle should not be changed by delayed focus'
+    )
+    assert.end()
+  })
+})
+
 // @todo
 // this test case needs some changes to the codebase in order to work
 // these changes will help testablity in general

src/router/router.js

@@ -406,33 +406,32 @@ const executeBeforeHook = async function (
     try {
       result = await hooks[hookName].call(parent, route, previousRoute)
       if (isString(result)) {
-        currentRoute = previousRoute
-        to(result)
+        this.currentRoute = previousRoute
+        to(result, {}, {}, this.name)
         return false
       }
     } catch (error) {
       Log.error(`Error or Rejected Promise in "${hookName}" Hook`, error)
+      this.currentRoute = previousRoute
       if (this.history.length > 0) {
         preventHashChangeNavigation = true
-        currentRoute = previousRoute
-        /// hmmmm
         window.history.back()
         navigatingBack = false
         state.navigating = false
-        return false
       }
+      return false
     }
     // If the resolved result is an object, redirect if the path in the object was changed
     if (isObject(result) === true && result.path !== currentPath) {
-      currentRoute = previousRoute
-      to(result.path, result.data, result.options)
+      this.currentRoute = previousRoute
+      to(result.path, result.data, result.options, this.name)
       return false
     }
     // If the resolved result is false, cancel navigation
     if (result === false) {
+      this.currentRoute = previousRoute
       if (this.history.length > 0) {
         preventHashChangeNavigation = true
-        currentRoute = previousRoute
         window.history.back()
         navigatingBack = false
         state.navigating = false

src/router/router.test.js

@@ -17,7 +17,7 @@
 
 import test from 'tape'
 import { initLog } from '../lib/log.js'
-import { to, navigate, back, state } from './router.js'
+import { back, navigate, state, to } from './router.js'
 import { matchHash, getHash, setHash } from './utils.js'
 import { stage } from '../launch.js'
 import Component from '../component.js'
@@ -863,6 +863,153 @@ test('BeforeEach hook route object redirect', async (assert) => {
   stage.element = originalElement
 })
 
+test('Cold redirect does not leave a phantom previous route', async (assert) => {
+  const originalElement = stage.element
+  stage.element = ({ parent }) => ({
+    populate() {},
+    set() {},
+    destroy() {},
+    parent,
+  })
+
+  const TestComponent = Component('ColdRedirectComponent', {
+    template: '<Element />',
+    code: { render: () => ({ elms: [], cleanup: () => {} }), effects: [] },
+  })
+
+  const host = {
+    [symbols.parent]: {
+      [symbols.routes]: [
+        {
+          path: '/cold-original',
+          component: TestComponent,
+          hooks: {
+            before() {
+              return '/cold-redirected'
+            },
+          },
+        },
+        { path: '/cold-redirected', component: TestComponent, options: { passFocus: false } },
+      ],
+    },
+    [symbols.children]: [{}],
+    [symbols.props]: {},
+    name: '',
+  }
+
+  to('/cold-original')
+  await navigate.call(host)
+  assert.equal(
+    host.currentRoute,
+    undefined,
+    'Redirect should restore the previous RouterView route'
+  )
+
+  await navigate.call(host)
+  assert.equal(host.currentRoute.path, '/cold-redirected', 'Redirected route should become current')
+  assert.equal(host[symbols.children].length, 2, 'Redirected view should remain attached')
+  assert.notEqual(host.activeView.eol, true, 'Redirected view should not be destroyed')
+
+  location.hash = '#/'
+  stage.element = originalElement
+})
+
+test('Named RouterView redirect retains named hash context', async (assert) => {
+  const originalElement = stage.element
+  stage.element = ({ parent }) => ({
+    populate() {},
+    set() {},
+    destroy() {},
+    parent,
+  })
+
+  const TestComponent = Component('NamedRedirectComponent', {
+    template: '<Element />',
+    code: { render: () => ({ elms: [], cleanup: () => {} }), effects: [] },
+  })
+
+  const host = {
+    [symbols.parent]: {
+      [symbols.routes]: [
+        {
+          path: '/named-original',
+          component: TestComponent,
+          hooks: {
+            before() {
+              return { path: '/named-redirected' }
+            },
+          },
+        },
+        { path: '/named-redirected', component: TestComponent, options: { passFocus: false } },
+      ],
+    },
+    [symbols.children]: [{}],
+    [symbols.props]: {},
+    name: 'modal',
+  }
+
+  location.hash = '#/'
+  to('/named-original', {}, {}, host.name)
+  await navigate.call(host)
+
+  assert.equal(
+    getHash(location.hash, 'modal').path,
+    '/named-redirected',
+    'Redirect should update the named RouterView hash segment'
+  )
+  assert.equal(getHash(location.hash).path, '/', 'Redirect should not replace the main route')
+
+  await navigate.call(host)
+  assert.equal(host.currentRoute.path, '/named-redirected', 'Named redirected route should render')
+  assert.equal(host[symbols.children].length, 2, 'Named redirected view should remain attached')
+
+  location.hash = '#/'
+  stage.element = originalElement
+})
+
+test('Rejected hook with empty history cancels navigation', async (assert) => {
+  const originalElement = stage.element
+  stage.element = ({ parent }) => ({
+    populate() {},
+    set() {},
+    destroy() {},
+    parent,
+  })
+
+  const TestComponent = Component('RejectedHookComponent', {
+    template: '<Element />',
+    code: { render: () => ({ elms: [], cleanup: () => {} }), effects: [] },
+  })
+
+  const host = {
+    [symbols.parent]: {
+      [symbols.routes]: [{ path: '/rejected-hook', component: TestComponent }],
+      [symbols.routerHooks]: {
+        beforeEach() {
+          throw new Error('Rejected hook')
+        },
+      },
+    },
+    [symbols.children]: [{}],
+    [symbols.props]: {},
+    name: '',
+  }
+
+  to('/rejected-hook')
+  await navigate.call(host)
+
+  assert.equal(
+    host.currentRoute,
+    undefined,
+    'Rejected navigation should restore the previous route'
+  )
+  assert.equal(host[symbols.children].length, 1, 'Rejected navigation should not render a view')
+  assert.equal(state.navigating, false, 'Rejected navigation should clear navigating state')
+
+  location.hash = '#/'
+  stage.element = originalElement
+})
+
 test('Route meta data is accessible in route object', (assert) => {
   const route = { path: '/test', meta: { auth: true, role: 'admin' } }
   assert.deepEqual(
@@ -1564,6 +1711,11 @@ test('beforeEach returning false leaves state.path on previous route', async (as
     '/guard-each-1',
     'Navigation should be cancelled when beforeEach returns false'
   )
+  assert.equal(
+    host.currentRoute.path,
+    '/guard-each-1',
+    'RouterView should restore its previous route'
+  )
 
   stage.element = originalElement
 })
@@ -1647,6 +1799,11 @@ test('route before returning false leaves state.path on previous route', async (
     '/guard-before-1',
     'Navigation should be cancelled when route before returns false'
   )
+  assert.equal(
+    host.currentRoute.path,
+    '/guard-before-1',
+    'RouterView should restore its previous route'
+  )
 
   stage.element = originalElement
 })