Repository: kyeotic/raviger
Pull request #65 (2020-08-28 03:32:50)

usePath unmounting
This PR is to track work regarding #64 

The current test case produces the following output
```
    console.log test/path.spec.js:144
      start harness update
    console.log test/path.spec.js:151
      harness update [Function: About]
    console.log test/path.spec.js:134
      about /about
    console.log test/path.spec.js:169
      reset
    console.log src/path.js:66
      changing path /
    console.log test/path.spec.js:134
      about /
    console.log src/path.js:66
      changing path /
    console.log test/path.spec.js:144
      start harness update
    console.log test/path.spec.js:151
      harness update [Function: Home]
    console.log test/path.spec.js:127
      home /
```
This behavior only shows up with the `navigate` call is in timeout, or some other async construct. I am not certain of the boundaries of this error, but synchronous navigation does not seem to reproduce it.

The logs reveals that the `useLocationChange` hook is firing first from the child component before the parent component containing the `useRoutes` call has a chance to force it to unmount.

Right now this seems to be a fairly significant architectural issue. I believe the correct behavior would be for `usePath` to work top-down somehow, giving the parent a chance to unmount the child. I am not sure if this would stop the popstate listener from firing though, since it has already been queued.

I don't currently have a plan for how to solve this.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bbd05cc..6e5721a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [UNRELEASED]
+### Fixed
+- `useRoutes` path tracking with `usePath` causing improper child invocations
+
 ## [1.4.5] - 2020-07-31
 ### Fixed
 - `navigate` handling of `replace` in edge cases for `replaceOrQuery`
diff --git a/package-lock.json b/package-lock.json
index 8b21afd..c40b737 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,6 +1,6 @@
 {
   "name": "raviger",
-  "version": "1.4.5",
+  "version": "1.4.6-0",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
diff --git a/package.json b/package.json
index e3ecd7d..7056184 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "raviger",
-  "version": "1.4.5",
+  "version": "1.4.6-0",
   "description": "React routing with hooks",
   "main": "dist/main.js",
   "module": "dist/module.js",
diff --git a/src/path.js b/src/path.js
index b40dd29..e806219 100644
--- a/src/path.js
+++ b/src/path.js
@@ -1,14 +1,22 @@
 import { useState, useEffect, useCallback, useRef, useContext } from 'react'
-import { BasePathContext } from './context.js'
+import { BasePathContext, PathContext } from './context.js'
 import { isNode, getSsrPath } from './node.js'
 import { isFunction } from './typeChecks.js'
 
 export function usePath(basePath) {
-  let contextBasePath = useBasePath() // hooks can't be called conditionally
+  const contextPath = useContext(PathContext)
+
+  const contextBasePath = useBasePath() // hooks can't be called conditionally
   basePath = basePath || contextBasePath
-  let [path, setPath] = useState(getCurrentPath(basePath))
+  const [path, setPath] = useState(getCurrentPath(basePath))
   useLocationChange(setPath, { basePath, inheritBasePath: !basePath })
-  return path
+
+  return (
+    (contextPath !== null
+      ? contextPath
+      : path.replace(basePathMatcher(basePath), '')) || '/'
+  )
+  // return path
 }
 
 export function useBasePath() {
diff --git a/src/router.js b/src/router.js
index 85aa1df..b876273 100644
--- a/src/router.js
+++ b/src/router.js
@@ -1,7 +1,7 @@
-import React, { useMemo } from 'react'
+import React, { useMemo, useState } from 'react'
 import { BasePathContext, PathContext } from './context.js'
 import { isNode, setSsrPath, getSsrPath } from './node'
-import { usePath } from './path.js'
+import { useLocationChange, getCurrentPath } from './path.js'
 
 export function useRoutes(
   routes,
@@ -13,7 +13,9 @@ export function useRoutes(
   } = {}
 ) {
   // path is the browser url location
-  const path = usePath(basePath)
+  const [path, setPath] = useState(getCurrentPath(basePath))
+
+  useLocationChange(setPath, { basePath, inheritBasePath: !basePath })
   // Get the current route
   const route = matchRoute(routes, path, {
     routeProps,


Test patch:
diff --git a/test/path.spec.js b/test/path.spec.js
index 3d41c64..f152572 100644
--- a/test/path.spec.js
+++ b/test/path.spec.js
@@ -1,5 +1,5 @@
-import React from 'react'
-import { render, act } from '@testing-library/react'
+import React, { useCallback, useEffect } from 'react'
+import { render, act, fireEvent } from '@testing-library/react'
 import {
   useRoutes,
   usePath,
@@ -119,6 +119,68 @@ describe('usePath', () => {
     act(() => navigate('/foo/nested/info'))
     expect(getByTestId('path')).toHaveTextContent('/')
   })
+
+  test('usePath is not called when unmounting', async () => {
+    const homeFn = jest.fn()
+    function Home() {
+      const path = usePath()
+      useEffect(() => {
+        // console.log('home', path)
+        homeFn(path)
+      }, [path])
+      return <span data-testid="path">{path}</span>
+    }
+    const aboutFn = jest.fn()
+    function About() {
+      const path = usePath()
+      useEffect(() => {
+        // console.log('about', path)
+        aboutFn(path)
+      }, [path])
+      return <span data-testid="path">{path}</span>
+    }
+
+    const routes = {
+      '/': () => <Home />,
+      '/about': () => <About />
+    }
+    function Harness({ routes, basePath }) {
+      // console.log('start harness update')
+      const route = useRoutes(routes, { basePath })
+      const onGoHome = useCallback(
+        () => setTimeout(() => navigate('/'), 50),
+        // () => setTimeout(() => act(() => navigate('/')), 50),
+        []
+      )
+      // console.log('harness update', route.props.children.props.children.type)
+      return (
+        <div>
+          {route}
+          <button data-testid="home-btn" onClick={onGoHome}>
+            Go Home
+          </button>
+        </div>
+      )
+    }
+
+    act(() => navigate('/about'))
+    const { getByTestId } = render(<Harness routes={routes} />)
+    expect(getByTestId('path')).toHaveTextContent('/about')
+    expect(aboutFn).toHaveBeenCalledTimes(1)
+
+    aboutFn.mockClear()
+
+    // console.log('reset')
+    // act(() => navigate('/'))
+    act(() => void fireEvent.click(getByTestId('home-btn')))
+    // console.log('acted')
+    // Wait for the internal setTimeout
+    await act(() => delay(100))
+
+    expect(getByTestId('path')).toHaveTextContent('/')
+    expect(homeFn).toHaveBeenCalledTimes(1)
+    expect(aboutFn).toHaveBeenCalledTimes(0)
+  })
 })
 
 describe('useHash', () => {
@@ -147,3 +209,7 @@ describe('useHash', () => {
     expect(getByTestId('hash')).toHaveTextContent('#test')
   })
 })
+
+async function delay(ms) {
+  return new Promise(resolve => setTimeout(() => resolve(), ms))
+}