Repository: kyeotic/raviger (ts)

Issue:
useLocationChange is called on mount
Hi @kyeotic!

We have just upgraded `raviger` from version `1.6.0` to version `2.0.2` and we are facing a problem with a new behaviour introduced in `2.0.0` in `useLocationChange`.

`useLocationChange` will now call its callback argument on mount due to this `useLayoutEffect`:
https://github.com/kyeotic/raviger/blob/74e3b87eaf108d0950ab09f368f1f91f23345dcb/src/path.js#L93-L98

Is that completely intended? Or should that be a layout effect that only gets executed after the initial mount?

Our use case for `useLocationChange` is within modals that we want to close when the user hits the Back/Forward browser history buttons:
```jsx
// close the modal when the path has changed
useLocationChange(onClose);
```

With `2.0.2`, this code gets executed when the modal is mounted and thus it closes instantly.

Pull request description:
default useLocationChange to skip initial render
closes #86
replaces #87

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ed328e0..72b219e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,11 @@ 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).
 
+## [2.1.0] - 2021-05-02
+### Added
+-  `options.onInitial` parameter for `useLocationChange` that controls the first render behavior. `default: false`.
+### Fixed
+- `useLocationChange` invoking the setter on initial render. This was not intended and was an unannounced change from the v1 behavior, so reverting it is not considered an API change but a bugfix.
 
 ## [2.0.2] - 2021-03-22
 ### Added
diff --git a/docs/api/useLocationChange.md b/docs/api/useLocationChange.md
index fba72a4..eb1c5c7 100644
--- a/docs/api/useLocationChange.md
+++ b/docs/api/useLocationChange.md
@@ -16,12 +16,15 @@ export function useLocationChange(
     inheritBasePath: boolean
     basePath: string
     isActive: () => boolean | boolean
+    onInitial?: boolean
   }
 ): void
 ```
 
 **Note**: `options.inheritBasePath` defaults to `true` (even if `options` is not provided), and takes precedence over `options.basePath` if `true`. If no BasePath is in the context to inherit `options.basePath` will be used as a fallback, if present. If `basePath` is provided, either by parameter or by context, and is missing from the current path `null` is sent to the `setFn` callback.
 
+By default this hook will not run on the initial mount for the component. You can get the location on the first render (mount) by setting `onInitial: true` in the `options` argument.
+
 ## 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).
diff --git a/docs/themes/learn b/docs/themes/learn
new file mode 160000
index 0000000..41c6cc5
--- /dev/null
+++ b/docs/themes/learn
@@ -0,0 +1,1 @@
+Subproject commit 41c6cc522f13ac324e99e8bf84f2b67d88510ceb
diff --git a/src/hooks.js b/src/hooks.js
new file mode 100644
index 0000000..7efae36
--- /dev/null
+++ b/src/hooks.js
@@ -0,0 +1,9 @@
+import { useLayoutEffect, useRef } from 'react'
+
+export function useMountedLayout(fn, deps, { onInitial = false } = {}) {
+  const hasMounted = useRef(onInitial)
+  useLayoutEffect(() => {
+    if (!hasMounted.current) hasMounted.current = true
+    else fn()
+  }, deps)
+}
diff --git a/src/path.js b/src/path.js
index 51bce41..63307fa 100644
--- a/src/path.js
+++ b/src/path.js
@@ -6,6 +6,7 @@ import {
   useLayoutEffect
 } from 'react'
 import { BasePathContext, PathContext } from './context.js'
+import { useMountedLayout } from './hooks.js'
 import { isNode, getSsrPath } from './node.js'
 import { isFunction } from './typeChecks.js'
 
@@ -62,7 +63,7 @@ export function getCurrentHash() {
 
 export function useLocationChange(
   setFn,
-  { inheritBasePath = true, basePath = '', isActive } = {}
+  { inheritBasePath = true, basePath = '', isActive, onInitial = false } = {}
 ) {
   if (isNode) return
   const routerBasePath = useBasePath()
@@ -92,10 +93,14 @@ export function useLocationChange(
 
   // When the basePath changes re-check the path after the render completes
   // This allows nested contexts to get an up-to-date formatted path
-  useLayoutEffect(() => {
-    if (isActive !== undefined && !isPredicateActive(isActive)) return
-    setRef.current(getFormattedPath(basePath))
-  }, [basePath, isActive])
+  useMountedLayout(
+    () => {
+      if (isActive !== undefined && !isPredicateActive(isActive)) return
+      setRef.current(getFormattedPath(basePath))
+    },
+    [basePath, isActive],
+    { onInitial }
+  )
 }
 
 /**


Test patch:
diff --git a/test/path.spec.js b/test/path.spec.js
index 456bec5..c042fc6 100644
--- a/test/path.spec.js
+++ b/test/path.spec.js
@@ -13,10 +13,22 @@ beforeEach(() => {
 })
 
 describe('useLocationChange', () => {
-  function Route({ onChange, isActive, basePath }) {
-    useLocationChange(onChange, { isActive, basePath })
+  function Route({ onChange, isActive, basePath, onInitial = false }) {
+    useLocationChange(onChange, { isActive, basePath, onInitial })
     return null
   }
+  test("setter doesn't get updated on mount", async () => {
+    let watcher = jest.fn()
+    render(<Route onChange={watcher} />)
+
+    expect(watcher).not.toBeCalled()
+  })
+  test('setter is updated on mount when onInitial is true', async () => {
+    let watcher = jest.fn()
+    render(<Route onChange={watcher} onInitial />)
+
+    expect(watcher).toBeCalled()
+  })
   test('setter gets updated path', async () => {
     let watcher = jest.fn()
     render(<Route onChange={watcher} />)