Repository: kyeotic/raviger (ts)

Issue:
Unnecessary re-renders using useQueryParams
Hey again @kyeotic 👋 

I am currently investigating some performance issues we are experiencing when changing URL parameters with `useQueryParams`.

I am finding issues within our own code but also what I think could be issues in `raviger`.

For instance, I have found this and I am not sure if it's an issue or a feature request: [Code Sandebox](https://codesandbox.io/s/strange-boyd-2ezbk)

Whenever setting the same values for the parameters, the `RouterContext.Provider` value changes, causing the components consuming the context to re-render.

The problem is here: [router.js#L32](https://github.com/kyeotic/raviger/blob/master/src/router.js#L32)

Each render loop generates a new object reference passed to the context provider, the value is not memoized with `useMemo`, and there is nothing that checks for deep equality of the parameters to prevent passing a new object reference either.

Additionally, pushing the same query params still pushes a new history state, and I am not sure if that is expected or a bug?

Pull request description:
reduce re-render thrashing
Fixes #41 

This is still a WIP. This eliminates _all but one_ unnecessary re-rendering when navigating to the same path, and eliminates all re-renders from nested components using `useQueryParams`-- that is, when `useRoutes` is a level above `useQueryParams` no re-rendering occurs when updating the query.

However, if you navigate to the same path you are on it stil re-renders once. I am not sure why.

Patch:
diff --git a/docs/content/api/useLocationChange.md b/docs/content/api/useLocationChange.md
index 1690711..d395266 100644
--- a/docs/content/api/useLocationChange.md
+++ b/docs/content/api/useLocationChange.md
@@ -20,7 +20,7 @@ export function useLocationChange(
 ): 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`.
+**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`. If no BasePath is in the context to inherit `options.basePath` will be used as a fallback, if present.
 
 ## Basic
 
diff --git a/example/index.html b/example/index.html
index cfe1422..7914d48 100644
--- a/example/index.html
+++ b/example/index.html
@@ -6,6 +6,7 @@
 	<title>Raviger</title>
 	<script src="/dist/react.js"></script>
 	<script src="/dist/react-dom.js"></script>
+	<script src="/dist/whyDidYouRender.js"></script>
 	<link href="/src/app.css" rel="stylesheet"></link>
 </head>
 
diff --git a/example/rollup.config.js b/example/rollup.config.js
index 9e00648..f6c8ee8 100644
--- a/example/rollup.config.js
+++ b/example/rollup.config.js
@@ -9,10 +9,11 @@ module.exports = {
     sourcemap: true,
     globals: {
       react: 'React',
-      'react-dom': 'ReactDOM'
+      'react-dom': 'ReactDOM',
+      whyDidYouRender: 'whyDidYouRender'
     }
   },
-  external: ['react', 'react-dom'],
+  external: ['react', 'react-dom', 'whyDidYouRender'],
   plugins: [
     replace({
       'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV)
diff --git a/example/src/App.js b/example/src/App.js
index b3877c5..b69267c 100644
--- a/example/src/App.js
+++ b/example/src/App.js
@@ -14,9 +14,12 @@ const routes = {
   '/*': () => <DisplayPath />
 }
 
-export default function App() {
+let renders = 0
+const App = () => {
+  renders++
   let route = useRoutes(routes)
   let path = usePath()
+  console.log('rendered', renders)
   return (
     <div>
       <Nav>
@@ -33,11 +36,18 @@ export default function App() {
       </Nav>
       <div></div>
       <span style={{ display: 'block' }}>Root Path: {path}</span>
+      <span style={{ display: 'block' }}>Render Count: {renders}</span>
       {route}
     </div>
   )
 }
 
+// App.whyDidYouRender = {
+//   logOnDifferentValues: true
+// }
+
+export default App
+
 const deepRoutes = {
   '/': () => <DisplayPath />,
   '/about': () => <DisplayPath />,
diff --git a/example/src/index.js b/example/src/index.js
index 923a290..91bb031 100644
--- a/example/src/index.js
+++ b/example/src/index.js
@@ -1,5 +1,9 @@
 import React from 'react'
 import ReactDOM from 'react-dom'
 import App from './App'
+// eslint-disable-next-line import/no-unresolved
+import whyDidYouRender from 'whyDidYouRender'
+
+whyDidYouRender(React)
 
 ReactDOM.render(<App />, document.getElementById('root'))
diff --git a/package-lock.json b/package-lock.json
index 1198e20..9fcd9f8 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,6 +1,6 @@
 {
   "name": "raviger",
-  "version": "1.2.0",
+  "version": "1.1.1",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
@@ -2559,6 +2559,15 @@
         "@xtuc/long": "4.2.2"
       }
     },
+    "@welldone-software/why-did-you-render": {
+      "version": "3.5.0",
+      "resolved": "https://registry.npmjs.org/@welldone-software/why-did-you-render/-/why-did-you-render-3.5.0.tgz",
+      "integrity": "sha512-9+WVSV8ZBb12SYu5uS4rOB5xVbW3ZpXGuAoeKlZ71oI9NrNG5X+v9WTFmoV+jKPKtlOU8dbwwDjF9GOzp7XvPQ==",
+      "dev": true,
+      "requires": {
+        "lodash": "^4"
+      }
+    },
     "@xtuc/ieee754": {
       "version": "1.2.0",
       "resolved": "https://registry.npmjs.org/@xtuc/ieee754/-/ieee754-1.2.0.tgz",
diff --git a/package.json b/package.json
index 8cafade..5182a6a 100644
--- a/package.json
+++ b/package.json
@@ -30,7 +30,7 @@
     "build:watch": "rollup -c --watch",
     "example:build": "NODE_ENV=development rollup -c example/rollup.config.js && yarn run example:build:link-react",
     "example:build:watch": "rollup -c example/rollup.config.js --watch",
-    "example:build:link-react": "mkdir -p example/dist && ln -sf $PWD/node_modules/react/umd/react.development.js example/dist/react.js && ln -sf $PWD/node_modules/react-dom/umd/react-dom.development.js example/dist/react-dom.js",
+    "example:build:link-react": "mkdir -p example/dist && ln -sf $PWD/node_modules/react/umd/react.development.js example/dist/react.js && ln -sf $PWD/node_modules/react-dom/umd/react-dom.development.js example/dist/react-dom.js && ln -sf $PWD/node_modules/@welldone-software/why-did-you-render/dist/umd/whyDidYouRender.js example/dist/whyDidYouRender.js",
     "example:serve": "npm run example:build:link-react && http-server example/ -p 8080 --proxy http://localhost:8080?",
     "example:dev": "run-p example:build:watch example:serve",
     "test": "jest",
@@ -44,6 +44,7 @@
     "size:check": "size-limit",
     "prepublishOnly": "run-s test:ci"
   },
+  "dependencies": {},
   "peerDependencies": {
     "react": "^16.8.0"
   },
@@ -55,6 +56,7 @@
     "@testing-library/jest-dom": "^4.2.4",
     "@testing-library/react": "^8.0.7",
     "@types/jest": "^24.0.25",
+    "@welldone-software/why-did-you-render": "^3.5.0",
     "babel-jest": "^24.8.0",
     "eslint": "^5.6.0",
     "eslint-plugin-jest": "^22.21.0",
diff --git a/src/context.js b/src/context.js
index c65fbfd..771354c 100644
--- a/src/context.js
+++ b/src/context.js
@@ -1,12 +1,14 @@
 import { createContext, useContext } from 'react'
 
-const RouterContext = createContext({
-  basePath: '',
-  path: null
-})
+const BasePathContext = createContext('')
+const PathContext = createContext(null)
 
-export default RouterContext
+export { BasePathContext }
+export { PathContext }
 
 export function useRouter() {
-  return useContext(RouterContext)
+  return {
+    basePath: useContext(BasePathContext),
+    path: useContext(PathContext)
+  }
 }
diff --git a/src/path.js b/src/path.js
index 7ecc86e..c56c93c 100644
--- a/src/path.js
+++ b/src/path.js
@@ -1,16 +1,18 @@
-import { useState, useEffect, useCallback, useRef } from 'react'
-import { useRouter } from './context.js'
+import { useState, useEffect, useCallback, useRef, useContext } from 'react'
+import { BasePathContext } from './context.js'
 import { isNode, getSsrPath } from './node.js'
 import { isFunction } from './typeChecks.js'
 
 export function usePath(basePath) {
-  let [path, setPath] = useState(getCurrentPath(basePath || useBasePath()))
+  let innerBasePath = useBasePath() // hooks can't be called conditionally
+  basePath = basePath || innerBasePath
+  let [path, setPath] = useState(getCurrentPath(basePath))
   useLocationChange(setPath, { basePath })
   return path
 }
 
 export function useBasePath() {
-  return useRouter().basePath
+  return useContext(BasePathContext)
 }
 
 export function getCurrentPath(basePath = '') {
@@ -33,7 +35,7 @@ export function useLocationChange(setFn, options = {}) {
   let basePath = ''
   const routerBasePath = useBasePath()
   if (options.inheritBasePath !== false) basePath = routerBasePath
-  else if (options.basePath) basePath = options.basePath
+  if (!basePath && options.basePath) basePath = options.basePath
   const setRef = useRef(setFn)
   useEffect(() => {
     // setFn could be an in-render declared callback, making it unstable
@@ -47,6 +49,7 @@ export function useLocationChange(setFn, options = {}) {
     // No predicate defaults true
     if (options.isActive !== undefined && !isPredicateActive(options.isActive))
       return
+    // console.log('tracking', getCurrentPath(basePath))
     setRef.current(getCurrentPath(basePath))
   }, [options.isActive, basePath])
   useEffect(() => {
diff --git a/src/querystring.js b/src/querystring.js
index a520c96..a6626a2 100644
--- a/src/querystring.js
+++ b/src/querystring.js
@@ -35,7 +35,7 @@ function parseQuery(querystring) {
 
 function serializeQuery(queryParams) {
   return Object.entries(queryParams).reduce((query, [key, value]) => {
-    query.append(key, value)
+    if (value !== null) query.append(key, value)
     return query
   }, new URLSearchParams())
 }
diff --git a/src/router.js b/src/router.js
index 855f33f..883c0d4 100644
--- a/src/router.js
+++ b/src/router.js
@@ -1,7 +1,7 @@
-import React, { useState, useMemo } from 'react'
-import RouterContext from './context.js'
+import React, { useMemo } from 'react'
+import { BasePathContext, PathContext } from './context.js'
 import { isNode, setSsrPath, getSsrPath } from './node'
-import { getCurrentPath, useLocationChange } from './path.js'
+import { usePath } from './path.js'
 
 export function useRoutes(
   routes,
@@ -13,13 +13,7 @@ export function useRoutes(
   } = {}
 ) {
   // path is the browser url location
-  const path = getCurrentPath(basePath)
-  const [context, setContext] = useState({ path })
-
-  // Watch for path changes
-  useLocationChange(path => setContext(context => ({ ...context, path })), {
-    basePath
-  })
+  const path = usePath(basePath)
   // Get the current route
   const route = matchRoute(routes, path, {
     routeProps,
@@ -29,9 +23,9 @@ export function useRoutes(
   // No match should not return an empty Provider, just null
   if (!route) return null
   return (
-    <RouterContext.Provider value={{ ...context, basePath }}>
-      {route}
-    </RouterContext.Provider>
+    <BasePathContext.Provider value={basePath}>
+      <PathContext.Provider value={path}>{route}</PathContext.Provider>
+    </BasePathContext.Provider>
   )
 }
 


Test patch:
diff --git a/test/querystring.spec.js b/test/querystring.spec.js
index 142b397..7e4af05 100644
--- a/test/querystring.spec.js
+++ b/test/querystring.spec.js
@@ -32,13 +32,10 @@ describe('useQueryParams', () => {
     expect(getByTestId('label')).toHaveTextContent(JSON.stringify(q1))
   })
   describe('setQueryParams', () => {
-    function Route({ replace }) {
+    function Route({ replace, foo = 'bar' }) {
       let [, setQuery] = useQueryParams()
       return (
-        <button
-          data-testid="update"
-          onClick={() => setQuery({ foo: 'bar' }, replace)}
-        >
+        <button data-testid="update" onClick={() => setQuery({ foo }, replace)}>
           Set Query
         </button>
       )
@@ -58,6 +55,14 @@ describe('useQueryParams', () => {
       expect(document.location.search).toContain('bar=foo')
       expect(document.location.search).toContain('foo=bar')
     })
+    test('merges query without null', async () => {
+      act(() => navigate('/about', { bar: 'foo' }))
+      const { getByTestId } = render(<Route replace={false} foo={null} />)
+
+      act(() => void fireEvent.click(getByTestId('update')))
+      expect(document.location.search).toContain('bar=foo')
+      expect(document.location.search).not.toContain('foo=')
+    })
     test('removes has when replace is true', async () => {
       act(() => navigate('/about#test'))
       const { getByTestId } = render(<Route replace />)