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(guards): run beforeRouteEnter with app context #2053

Closed
wants to merge 3 commits into from

Conversation

posva
Copy link
Member

@posva posva commented Nov 22, 2023

Fix #2051

Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit 66711f1
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/655f1098d02aec00089382a3

@posva
Copy link
Member Author

posva commented Nov 22, 2023

CI seems broken on the PR but works locally...

@rijenkii
Copy link

Modified packages/router/e2e/guards-instances/index.ts to output hasInjectionContext into logs and asynchronously loading view components, but it seems to fail:

Patch
diff --git a/packages/router/e2e/guards-instances/index.ts b/packages/router/e2e/guards-instances/index.ts
index 2a4aa04..8ff8ece 100644
--- a/packages/router/e2e/guards-instances/index.ts
+++ b/packages/router/e2e/guards-instances/index.ts
@@ -7,7 +7,14 @@ import {
   useRoute,
   useRouter,
 } from 'vue-router'
-import { createApp, ref, reactive, defineComponent, computed } from 'vue'
+import {
+  createApp,
+  ref,
+  reactive,
+  defineComponent,
+  computed,
+  hasInjectionContext,
+} from 'vue'
 import { isArray } from '../../src/utils'
 
 // override existing style on dev with shorter times
@@ -65,7 +72,11 @@ function createTestComponent(key: string) {
     // },
     beforeRouteEnter(to, from, next) {
       state.enter++
-      logs.value.push(`${key}: enter ${from.path} - ${to.path}`)
+      logs.value.push(
+        `${key}: enter ${from.path} - ${
+          to.path
+        }, hasInjectionContext=${hasInjectionContext()}`
+      )
       next(vm => {
         // @ts-expect-error
         vm.enterCallback++
@@ -75,21 +86,37 @@ function createTestComponent(key: string) {
       if (!this || this.key !== key) throw new Error('no this')
       state.update++
       this.selfUpdates++
-      logs.value.push(`${key}: update ${from.path} - ${to.path}`)
+      logs.value.push(
+        `${key}: update ${from.path} - ${
+          to.path
+        }, hasInjectionContext=${hasInjectionContext()}`
+      )
     },
     beforeRouteLeave(to, from) {
       if (!this || this.key !== key) throw new Error('no this')
       state.leave++
       this.selfLeaves++
-      logs.value.push(`${key}: leave ${from.path} - ${to.path}`)
+      logs.value.push(
+        `${key}: leave ${from.path} - ${
+          to.path
+        }, hasInjectionContext=${hasInjectionContext()}`
+      )
     },
 
     setup() {
       onBeforeRouteUpdate((to, from) => {
-        logs.value.push(`${key}: setup:update ${from.path} - ${to.path}`)
+        logs.value.push(
+          `${key}: setup:update ${from.path} - ${
+            to.path
+          }, hasInjectionContext=${hasInjectionContext()}`
+        )
       })
       onBeforeRouteLeave((to, from) => {
-        logs.value.push(`${key}: setup:leave ${from.path} - ${to.path}`)
+        logs.value.push(
+          `${key}: setup:leave ${from.path} - ${
+            to.path
+          }, hasInjectionContext=${hasInjectionContext()}`
+        )
       })
       return {}
     },
@@ -109,33 +136,33 @@ const webHistory = createWebHistory('/guards-instances')
 const router = createRouter({
   history: webHistory,
   routes: [
-    { path: '/', component: Home },
+    { path: '/', component: async () => Home },
     {
       path: '/foo',
-      component: Foo,
+      component: async () => Foo,
     },
     {
       path: '/f/:id',
-      component: Foo,
+      component: async () => Foo,
     },
     // TODO: test that the onBeforeRouteUpdate isn't kept
     {
       path: '/b/:id',
       name: 'id',
-      component: WithId,
+      component: async () => WithId,
     },
     {
       path: '/named-one',
       components: {
-        default: One,
-        aux: Aux,
+        default: async () => One,
+        aux: async () => Aux,
       },
     },
     {
       path: '/named-two',
       components: {
-        default: Two,
-        aux: Aux,
+        default: async () => Two,
+        aux: async () => Aux,
       },
     },
   ],
diff --git a/packages/router/e2e/specs/guards-instances.js b/packages/router/e2e/specs/guards-instances.js
index b2394e4..35b5976 100644
--- a/packages/router/e2e/specs/guards-instances.js
+++ b/packages/router/e2e/specs/guards-instances.js
@@ -23,17 +23,17 @@ function testCase(browser, name) {
     .expect.element('#logs')
     .text.to.equal(
       [
-        `${name}: enter / - /foo`,
-        `${name}: leave /foo - /f/1`,
-        `${name}: setup:leave /foo - /f/1`,
-        `${name}: enter /foo - /f/1`,
-        `${name}: update /f/1 - /f/2`,
-        `${name}: setup:update /f/1 - /f/2`,
-        `${name}: update /f/2 - /f/2`,
-        `${name}: setup:update /f/2 - /f/2`,
-        `${name}: leave /f/2 - /foo`,
-        `${name}: setup:leave /f/2 - /foo`,
-        `${name}: enter /f/2 - /foo`,
+        `${name}: enter / - /foo, hasInjectionContext=true`,
+        `${name}: leave /foo - /f/1, hasInjectionContext=true`,
+        `${name}: setup:leave /foo - /f/1, hasInjectionContext=true`,
+        `${name}: enter /foo - /f/1, hasInjectionContext=true`,
+        `${name}: update /f/1 - /f/2, hasInjectionContext=true`,
+        `${name}: setup:update /f/1 - /f/2, hasInjectionContext=true`,
+        `${name}: update /f/2 - /f/2, hasInjectionContext=true`,
+        `${name}: setup:update /f/2 - /f/2, hasInjectionContext=true`,
+        `${name}: leave /f/2 - /foo, hasInjectionContext=true`,
+        `${name}: setup:leave /f/2 - /foo, hasInjectionContext=true`,
+        `${name}: enter /f/2 - /foo, hasInjectionContext=true`,
       ].join('\n')
     )
 }
@@ -126,10 +126,10 @@ module.exports = {
       .expect.element('#logs')
       .text.to.equal(
         [
-          `${name}: update /f/1 - /f/2`,
-          `${name}: setup:update /f/1 - /f/2`,
-          `${name}: leave /f/2 - /`,
-          `${name}: setup:leave /f/2 - /`,
+          `${name}: update /f/1 - /f/2, hasInjectionContext=true`,
+          `${name}: setup:update /f/1 - /f/2, hasInjectionContext=true`,
+          `${name}: leave /f/2 - /, hasInjectionContext=true`,
+          `${name}: setup:leave /f/2 - /, hasInjectionContext=true`,
         ].join('\n')
       )
 
@@ -175,8 +175,8 @@ module.exports = {
       .text.to.equal(
         [
           // to force new lines formatting
-          `${name}: update /f/2 - /f/2`,
-          `${name}: setup:update /f/2 - /f/2`,
+          `${name}: update /f/2 - /f/2, hasInjectionContext=true`,
+          `${name}: setup:update /f/2 - /f/2, hasInjectionContext=true`,
         ].join('\n')
       )
     browser
@@ -238,8 +238,8 @@ module.exports = {
       .text.to.equal(
         [
           // foo
-          `${name}: update /f/2 - /f/2`,
-          `${name}: setup:update /f/2 - /f/2`,
+          `${name}: update /f/2 - /f/2, hasInjectionContext=true`,
+          `${name}: setup:update /f/2 - /f/2, hasInjectionContext=true`,
         ].join('\n')
       )
     browser
@@ -250,10 +250,10 @@ module.exports = {
       .expect.element('#logs')
       .text.to.equal(
         [
-          `${name}: update /f/2 - /f/2`,
-          `${name}: setup:update /f/2 - /f/2`,
-          `${name}: update /f/2 - /f/2`,
-          `${name}: setup:update /f/2 - /f/2`,
+          `${name}: update /f/2 - /f/2, hasInjectionContext=true`,
+          `${name}: setup:update /f/2 - /f/2, hasInjectionContext=true`,
+          `${name}: update /f/2 - /f/2, hasInjectionContext=true`,
+          `${name}: setup:update /f/2 - /f/2, hasInjectionContext=true`,
         ].join('\n')
       )
 
@@ -271,12 +271,12 @@ module.exports = {
       .expect.element('#logs')
       .text.to.equal(
         [
-          `One: enter / - /named-one`,
-          `Aux: enter / - /named-one`,
-          `One: leave /named-one - /`,
-          `Aux: leave /named-one - /`,
-          `One: setup:leave /named-one - /`,
-          `Aux: setup:leave /named-one - /`,
+          `One: enter / - /named-one, hasInjectionContext=true`,
+          `Aux: enter / - /named-one, hasInjectionContext=true`,
+          `One: leave /named-one - /, hasInjectionContext=true`,
+          `Aux: leave /named-one - /, hasInjectionContext=true`,
+          `One: setup:leave /named-one - /, hasInjectionContext=true`,
+          `Aux: setup:leave /named-one - /, hasInjectionContext=true`,
         ].join('\n')
       )
 
@@ -287,12 +287,12 @@ module.exports = {
       .expect.element('#logs')
       .text.to.equal(
         [
-          `One: leave /named-one - /named-two`,
-          `Aux: leave /named-one - /named-two`,
-          `One: setup:leave /named-one - /named-two`,
-          `Aux: setup:leave /named-one - /named-two`,
-          `Two: enter /named-one - /named-two`,
-          `Aux: enter /named-one - /named-two`,
+          `One: leave /named-one - /named-two, hasInjectionContext=true`,
+          `Aux: leave /named-one - /named-two, hasInjectionContext=true`,
+          `One: setup:leave /named-one - /named-two, hasInjectionContext=true`,
+          `Aux: setup:leave /named-one - /named-two, hasInjectionContext=true`,
+          `Two: enter /named-one - /named-two, hasInjectionContext=true`,
+          `Aux: enter /named-one - /named-two, hasInjectionContext=true`,
         ].join('\n')
       )
pnpm run -r test:e2e output
│   ️TEST FAILURE (40.598s):  
│    - 1 assertions failed; 259 passed
│    - 6 skipped
│    ✖ 1) guards-instances
│    – guards instances (5.739s)
│    → ✖ NightwatchAssertError
│    Expected element <#logs> text to equal: "Foo: enter / - /foo, hasInjectionContext=true
│ Foo: leave /foo - /f/1, hasInjectionContext=true
│ Foo: setup:leave /foo - /f/1, hasInjectionContext=true
│ Foo: enter /foo - /f/1, hasInjectionContext=true
│ Foo: update /f/1 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/1 - /f/2, hasInjectionContext=true
│ Foo: update /f/2 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/2 - /f/2, hasInjectionContext=true
│ Foo: leave /f/2 - /foo, hasInjectionContext=true
│ Foo: setup:leave /f/2 - /foo, hasInjectionContext=true
│ Foo: enter /f/2 - /foo, hasInjectionContext=true" - expected "equal 'Foo: enter / - /foo, hasInjectionContext=true
│ Foo: leave /foo - /f/1, hasInjectionContext=true
│ Foo: setup:leave /foo - /f/1, hasInjectionContext=true
│ Foo: enter /foo - /f/1, hasInjectionContext=true
│ Foo: update /f/1 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/1 - /f/2, hasInjectionContext=true
│ Foo: update /f/2 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/2 - /f/2, hasInjectionContext=true
│ Foo: leave /f/2 - /foo, hasInjectionContext=true
│ Foo: setup:leave /f/2 - /foo, hasInjectionContext=true
│ Foo: enter /f/2 - /foo, hasInjectionContext=true'" but got: "Foo: enter / - /foo, hasInjectionContext=false
│ Foo: leave /foo - /f/1, hasInjectionContext=true
│ Foo: setup:leave /foo - /f/1, hasInjectionContext=true
│ Foo: enter /foo - /f/1, hasInjectionContext=false
│ Foo: update /f/1 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/1 - /f/2, hasInjectionContext=true
│ Foo: update /f/2 - /f/2, hasInjectionContext=true
│ Foo: setup:update /f/2 - /f/2, hasInjectionContext=true
│ Foo: leave /f/2 - /foo, hasInjectionContext=true
│ Foo: setup:leave /f/2 - /foo, hasInjectionContext=true
│ Foo: enter /f/2 - /foo, hasInjectionContext=true" (5104ms)
│ 
│     Error location:
│     /tmp/router/packages/router/e2e/specs/guards-instances.js:
│     –––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
│      21 |     .assert.textContains(`#${name} .update`, '2')
│      22 |     .assert.textContains(`#${name} .leave`, '2')
│      23 |     .expect.element('#logs') 
│      24 |     .text.to.equal(
│      25 |       [
│     –––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
│ 
│     SKIPPED (at runtime):
│     - cancel pending pop navigations
│     - guards instances transition
│     - guards instances keep alive
│     - guards instances keyed
│     - guards instances keepalive keyed
│     - guards + instances + named views
│  Wrote HTML report file to: /tmp/router/packages/router/tests_output/nightwatch-html-report/index.html
│ 
│  ELIFECYCLE  Command failed with exit code 5.
└─ Failed in 41.3s at /tmp/router/packages/router

@posva
Copy link
Member Author

posva commented Nov 23, 2023

Thanks for the patch! We can go with a unit test instead (I pushed the failing test). What I added doesn't fix the issue.

Feel free to give it a try and even open a new PR based on this one. It doesn't matter if you keep the commits or not 🙂

@posva posva closed this in #2117 Jan 29, 2024
@posva posva deleted the fix/2051-beforeRouteEnter-context branch February 13, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

beforeRouteEnter does not have an injection context if component is not yet loaded
2 participants