Repository: kyeotic/raviger (ts)

Issue:
usePopState has wrong type definition, hook dependencies
Hey again!

So I have a use-case where a modal dialog contains links and the modal needs to close on any history change.

Rather than making my own listener, I went to see how `raviger` implements it, and found the nifty `usePopState` hook that does pretty much what I need. However I found the following:

1. `usePopState` is exposed but has no entry in the documentation. Is it part of the API? Is it fine to use it?

2. In the type definition, [it says](https://github.com/kyeotic/raviger/blob/master/types/raviger.d.ts#L44) that the `predicate` parameter is a function that returns a boolean, however, according to [the actual implementation](https://github.com/kyeotic/raviger/blob/master/src/path.js#L24) and usages across the library, it seems that it's only expecting a boolean.

3. The `predicate` parameter is not added to the dependencies of the `useEffect` hook, which means that if we need to change that condition after the initial render, it won't be taken into account.

4. I feel that `predicate` should be an optional argument, and could be moved to being the third parameter?

5. Probably have the same feeling about the first parameter. Seems like `basePath` could be optional and default to `''`.

Looking forward to hearing about your opinion on these 👍 

Pull request:
usePopState -> useLocationChange
rename `usePopState` to `useLocationChange` and rework the parameters.

Fixes #28 



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b2c1639..b733d4f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,12 @@ 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).
 
+## [1.0.0] - UNRELEASED
+### Added
+- `useLocationChange`: similar to use `usePopState`, but uses different parameters
+### Removed
+- `usePopState`
+
 ## [0.5.9] - 2019-10-28
 ### Added
 - `useNavigationPrompt` for confirming navigation changes
diff --git a/docs/README.md b/docs/README.md
index b537fe8..8ee91f5 100644
--- a/docs/README.md
+++ b/docs/README.md
@@ -9,4 +9,13 @@ You may need to run this command once to initialize the theme submodule
 
 ```bash
 git submodule sync && git submodule update --init
+```
+
+# Creating Content
+
+```bash
+# Either
+hugo new <chapter>/<name>/_index.md
+# Or
+hugo new <chapter>/<name>.md
 ```
\ No newline at end of file
diff --git a/docs/content/api/useLocationChange.md b/docs/content/api/useLocationChange.md
new file mode 100644
index 0000000..1690711
--- /dev/null
+++ b/docs/content/api/useLocationChange.md
@@ -0,0 +1,79 @@
+---
+title: "useLocationChange"
+date: 2019-11-05T09:44:14-08:00
+weight: 10
+---
+
+
+This hook invokes a setter whenever the page location is updated.
+
+## API
+
+{{< highlight typescript >}}
+export function useLocationChange(
+  setFn: (path: string) => any,
+  options?: {
+    inheritBasePath: boolean
+    basePath: string
+    isActive: () => boolean | boolean
+  }
+): void
+{{< /highlight >}}
+
+**Note**: `options.inheritBasePath` is treated as `true` unless it is set to `false` (even if `options` is not provided), and takes precedence over `options.basePath` if `true`.
+
+## Basic
+
+The first parameter is a setter-function that is invoked with the new path whenever the url is changed. It does not automatically cause a re-render of the parent component (see _re-rendering_ below).
+
+{{< highlight jsx>}}
+import { useLocationChange } from 'raviger'
+import { pageChanged } from './monitoring'
+
+function App () {
+  useLocationChange(pageChanged)
+
+  return (
+    // ...
+  )
+}
+{{< /highlight>}}
+
+You should try to provide the same function (referential-equality) to **useLocationChange** whenever possible. If you are unable to create the function outside the component scope use **useCallback** to get a memoized function.
+
+{{< highlight jsx>}}
+import { useCallback } from 'react'
+import { useLocationChange } from 'raviger'
+import { pageChanged } from './monitoring'
+
+function App () {
+  const onChange = useCallback(path => pageChanged('App', path), [])
+  useLocationChange(onChange)
+
+  return (
+    // ...
+  )
+}
+{{< /highlight>}}
+
+## Conditional Updates
+
+When `options.isActive` is both **defined** and **falsey** the `setFn` will not be invoked during location changes. If it is **undefined** or **truthy** `setFn` will be invoked.
+
+## Re-rendering
+
+**useLocationChange** does not itself cause re-rendering. If you are trying to get the current path in your component that is better done with the [usePath](/api/link) hook, which returns the value. However, it is possible to trigger re-rendering by combining **useLocationChange** with **useState**.
+
+{{< highlight jsx>}}
+import { useState } from 'react'
+import { useLocationChange } from 'raviger'
+
+function Route () {
+  const [path, setPath] = useState('/')
+  useLocationChange(setPath)
+
+  return (
+    // ...
+  )
+}
+{{< /highlight>}}
diff --git a/package-lock.json b/package-lock.json
index ea8bf8a..2bb6dfa 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,6 +1,6 @@
 {
   "name": "raviger",
-  "version": "0.5.9",
+  "version": "1.0.0-rc.1",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
diff --git a/package.json b/package.json
index 177a274..a79012d 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "raviger",
-  "version": "0.5.9",
+  "version": "1.0.0-rc.1",
   "description": "React routing with hooks",
   "main": "dist/main.js",
   "module": "dist/module.js",
diff --git a/src/main.js b/src/main.js
index 8f660e5..c8afae9 100644
--- a/src/main.js
+++ b/src/main.js
@@ -2,5 +2,5 @@ export { useRoutes } from './router.js'
 export { useRedirect } from './redirect.js'
 export { default as Link, ActiveLink } from './Link.js'
 export { navigate, useNavigationPrompt } from './navigate.js'
-export { usePath, useBasePath, usePopState } from './path.js'
+export { usePath, useBasePath, useLocationChange } from './path.js'
 export { useQueryParams } from './querystring.js'
diff --git a/src/path.js b/src/path.js
index 6844927..acac4c8 100644
--- a/src/path.js
+++ b/src/path.js
@@ -1,11 +1,15 @@
 import { useState, useEffect } from 'react'
 import { useRouter } from './context.js'
 import { isNode, getSsrPath } from './node.js'
+import { isFunction } from './typeChecks.js'
 
 export function usePath(basePath) {
   let context = useRouter()
   let [path, setPath] = useState(context.path || getCurrentPath(basePath))
-  usePopState(basePath, isNode || context.path, setPath)
+  useLocationChange(setPath, {
+    basePath,
+    isActive: !context.path
+  })
   return context.path || path
 }
 
@@ -19,15 +23,26 @@ export function getCurrentPath(basePath = '') {
     : window.location.pathname.replace(basePathMatcher(basePath), '') || '/'
 }
 
-export function usePopState(basePath, predicate, setFn) {
+export function useLocationChange(setFn, options = {}) {
+  if (isNode) return
+  let basePath = ''
+  const routerBasePath = useBasePath()
+  if (options.inheritBasePath !== false) basePath = routerBasePath
+  else if (options.basePath) basePath = options.basePath
   useEffect(() => {
-    if (predicate) return
+    // No predicate defaults true
+    if (options.isActive !== undefined && !isPredicateActive(options.isActive))
+      return
     const onPopState = () => {
       setFn(getCurrentPath(basePath))
     }
     window.addEventListener('popstate', onPopState)
     return () => window.removeEventListener('popstate', onPopState)
-  }, [basePath])
+  }, [setFn, options.isActive, basePath])
+}
+
+function isPredicateActive(predicate) {
+  return isFunction(predicate) ? predicate() : predicate
 }
 
 function basePathMatcher(basePath) {
diff --git a/src/querystring.js b/src/querystring.js
index 466e681..90a132d 100644
--- a/src/querystring.js
+++ b/src/querystring.js
@@ -1,7 +1,7 @@
 import { useState, useCallback } from 'react'
 import { isNode, getSsrPath } from './node.js'
 import { navigate } from './navigate.js'
-import { getCurrentPath, usePopState } from './path.js'
+import { getCurrentPath, useLocationChange } from './path.js'
 
 export function useQueryParams(
   parseFn = parseQuery,
@@ -16,7 +16,7 @@ export function useQueryParams(
     [querystring]
   )
   // Watch for location changes
-  usePopState('', isNode, () => setQuerystring(getQueryString()))
+  useLocationChange(() => setQuerystring(getQueryString()))
   return [parseFn(querystring), setQueryParams]
 }
 
diff --git a/src/router.js b/src/router.js
index 77d62b5..02055cc 100644
--- a/src/router.js
+++ b/src/router.js
@@ -1,7 +1,7 @@
-import React, { useState, useMemo } from 'react'
+import React, { useState, useMemo, useCallback } from 'react'
 import RouterContext from './context.js'
 import { isNode, setSsrPath, getSsrPath } from './node'
-import { getCurrentPath, usePopState } from './path.js'
+import { getCurrentPath, useLocationChange } from './path.js'
 
 export function useRoutes(
   routes,
@@ -17,9 +17,14 @@ export function useRoutes(
   const [context, setContext] = useState({ path })
 
   // Watch for location changes
-  usePopState(basePath, isNode, path =>
-    setContext(context => ({ ...context, path }))
+  const pathWatcher = useCallback(
+    path => setContext(context => ({ ...context, path })),
+    []
   )
+  useLocationChange(pathWatcher, {
+    basePath
+  })
+  // Get the current route
   const route = matchRoute(routes, path, {
     routeProps,
     overridePathParams,
diff --git a/src/typeChecks.js b/src/typeChecks.js
new file mode 100644
index 0000000..26e1c6a
--- /dev/null
+++ b/src/typeChecks.js
@@ -0,0 +1,3 @@
+export function isFunction(obj) {
+  return obj && typeof obj === 'function'
+}
diff --git a/types/raviger.d.ts b/types/raviger.d.ts
index 4089a86..69a6945 100644
--- a/types/raviger.d.ts
+++ b/types/raviger.d.ts
@@ -39,10 +39,13 @@ export function usePath(basePath?: string): string
 
 export function useBasePath(): string
 
-export function usePopState(
-  basePath: string,
-  predicate: () => boolean,
-  setFn: (path: string) => any
+export function useLocationChange(
+  setFn: (path: string) => any,
+  options?: {
+    inheritBasePath?: boolean
+    basePath?: string
+    isActive?: () => boolean | boolean
+  }
 ): void
 
 export interface QueryParam {


Test patch:
diff --git a/test/path.spec.js b/test/path.spec.js
new file mode 100644
index 0000000..c440e01
--- /dev/null
+++ b/test/path.spec.js
@@ -0,0 +1,28 @@
+import React from 'react'
+import { render, act } from '@testing-library/react'
+import { useLocationChange, navigate } from '../src/main.js'
+
+beforeAll(() => {
+  act(() => navigate('/'))
+})
+
+describe('useLocationChange', () => {
+  function Route({ onChange, isActive }) {
+    useLocationChange(onChange, { isActive })
+    return null
+  }
+  test('setter gets updated path', async () => {
+    let watcher = jest.fn()
+    render(<Route onChange={watcher} />)
+    act(() => navigate('/foo'))
+
+    expect(watcher).toBeCalledWith('/foo')
+  })
+  test('setter is not updated when isActive is false', async () => {
+    let watcher = jest.fn()
+    render(<Route onChange={watcher} isActive={false} />)
+    act(() => navigate('/foo'))
+
+    expect(watcher).not.toBeCalled()
+  })
+})
diff --git a/test/router.spec.js b/test/router.spec.js
index ac9cddc..3c33cbb 100644
--- a/test/router.spec.js
+++ b/test/router.spec.js
@@ -8,7 +8,7 @@ import {
   navigate
 } from '../src/main.js'
 
-afterAll(() => {
+beforeAll(() => {
   act(() => navigate('/'))
 })