Fix location state (#394) #399

BlazPocrnja · 2020-03-09T20:57:07Z

Closes #394. This change deep compares location state objects to prevent incorrectly pushing to history multiple times when state is changed.

Ran all tests

supasate · 2020-03-14T17:10:41Z

src/ConnectedRouter.js

@@ -2,6 +2,7 @@ import React, { PureComponent } from 'react'
 import PropTypes from 'prop-types'
 import { connect, ReactReduxContext } from 'react-redux'
 import { Router } from 'react-router'
+import { isEqual } from 'lodash'


import isEqual from 'lodash.isequal' to avoid loading the whole lodash module.

Please run yarn add -P lodash.isequal to update package.json and yarn.lock as peerDependencies.

supasate · 2020-03-14T17:11:08Z

Hi @BlazPocrnja , would you mind adding a unit test for this behavior?

BlazPocrnja · 2020-03-16T20:33:02Z

@supasate I added unit tests and changed the lodash function to isEqualWith to allow for custom comparator functions as per this comment #394 (comment).

supasate · 2020-03-17T03:05:34Z

src/ConnectedRouter.js

@@ -37,15 +38,15 @@ const createConnectedRouter = (structure) => {
          search: searchInHistory,
          hash: hashInHistory,
          state: stateInHistory,
-        } = history.location
+				} = history.location


nit: use spaces, not tabs.

supasate · 2020-03-17T03:06:08Z

src/ConnectedRouter.js

@@ -104,7 +105,8 @@ const createConnectedRouter = (structure) => {
    }).isRequired,
    basename: PropTypes.string,
    children: PropTypes.oneOfType([ PropTypes.func, PropTypes.node ]),
-    onLocationChanged: PropTypes.func.isRequired,
+		onLocationChanged: PropTypes.func.isRequired,


ditto: spaces

supasate · 2020-03-17T03:06:20Z

src/ConnectedRouter.js

+		onLocationChanged: PropTypes.func.isRequired,
+		stateCompareFunction: PropTypes.func,


ditto: spaces

supasate · 2020-03-17T03:07:54Z

src/ConnectedRouter.js

@@ -18,7 +19,7 @@ const createConnectedRouter = (structure) => {
    constructor(props) {
      super(props)

-      const { store, history, onLocationChanged } = props
+      const { store, history, onLocationChanged, stateCompareFunction} = props


nit: add a space before the closing curling brace.

supasate · 2020-03-17T03:12:28Z

test/ConnectedRouter.test.js

+			})
+
+			expect(props.history.entries).toHaveLength(3)


supasate · 2020-03-17T03:12:47Z

test/ConnectedRouter.test.js

+			})
+
+			expect(props.history.entries).toHaveLength(3)


ditto: spaces

supasate · 2020-03-17T03:15:57Z

test/ConnectedRouter.test.js

+      store.dispatch({
+        type: LOCATION_CHANGE,
+        payload: {
+          location: {
+            pathname: '/',
+            search: '',
+            hash: '',
+            state: { foo: 'baz' }
+          },
+          action: 'PUSH',
+        }
+      })
+
+      expect(props.history.entries).toHaveLength(4)
+    })


Can you split this into another test? So, we have one for testing that it doesn't update if the location state structure doesn't change, and another one for testing that it updates if the location state structure changes.

supasate · 2020-03-17T03:18:41Z

test/ConnectedRouter.test.js

+          },
+          action: 'PUSH',
+        }
+			})


supasate · 2020-03-17T03:19:02Z

test/ConnectedRouter.test.js

+							return a === undefined  || (a.foo === "baz" && b.foo === 'bar') ? true : false
+            }}
+            {...props}
+					>


supasate · 2020-03-17T03:22:29Z

test/ConnectedRouter.test.js

+        <Provider store={store}>
+          <ConnectedRouter
+            stateCompareFunction={(a, b) => {
+							return a === undefined  || (a.foo === "baz" && b.foo === 'bar') ? true : false


No need to add a === undefined which we doesn't test it. Only the second comparison is enough to test this comparison feature.

Suggested change

return a === undefined || (a.foo === "baz" && b.foo === 'bar') ? true : false

return (a.foo === 'baz' && b.foo === 'bar') ? true : false

supasate

Looks good to me. Please rebase and fix some nits.

…ion test + spacing

BlazPocrnja · 2020-03-21T22:27:46Z

@supasate Sorry for the delay! I made the changes you requested. Except when testing the custom state comparator function, there needs to be a check when the states are undefined (which is the case when you first push to history, or the store). I rewrote the test to make it a bit clearer. It's rebased now too!

supasate · 2020-03-22T23:44:54Z

LGTM. Thanks for your contribution @BlazPocrnja !

supasate · 2020-03-23T00:26:02Z

I just noticed that it breaks the test because lodash.isequalwith is not listed in devDependencies. (https://travis-ci.org/github/supasate/connected-react-router/builds/665324580?utm_medium=notification&utm_source=github_status). I'm not sure why Travis didn't show up here. I'm fixing in #408.

BlazPocrnja · 2020-03-23T00:33:57Z

My apologies! Looks like it's passing now, so that's a relief. Thanks!

BlazPocrnja mentioned this pull request Mar 12, 2020

V6.7.0 breaks the location state flow #394

Closed

supasate requested changes Mar 14, 2020

View reviewed changes

BlazPocrnja force-pushed the master branch from 9286e28 to 67bc5d7 Compare March 16, 2020 18:40

supasate reviewed Mar 17, 2020

View reviewed changes

test/ConnectedRouter.test.js Outdated

Comment on lines 184 to 186

})

expect(props.history.entries).toHaveLength(3)

Copy link

Owner

supasate Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

supasate reviewed Mar 17, 2020

View reviewed changes

test/ConnectedRouter.test.js Outdated

Comment on lines 169 to 171

})

expect(props.history.entries).toHaveLength(3)

Copy link

Owner

supasate Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: spaces

supasate reviewed Mar 17, 2020

View reviewed changes

test/ConnectedRouter.test.js Outdated

},

action: 'PUSH',

}

})

Copy link

Owner

supasate Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

supasate reviewed Mar 17, 2020

View reviewed changes

supasate requested changes Mar 17, 2020

View reviewed changes

BlazPocrnja added 6 commits March 21, 2020 16:10

Fix location state (supasate#394)

93f4404

Changed to isEqualWith with optional comparator prop (supasate#394)

982a8e2

Added unit tests

d22202f

Fixed file text tabs and spacing

e68defe

Added more expected outputs

6dd8265

Split store update test into two + updated custom state compare funct…

7edaa29

…ion test + spacing

BlazPocrnja force-pushed the master branch from 09db27e to 7edaa29 Compare March 21, 2020 22:14

Resolved git rebasing issue

4daadeb

supasate approved these changes Mar 22, 2020

View reviewed changes

supasate merged commit 785db5c into supasate:master Mar 22, 2020

supasate mentioned this pull request Mar 23, 2020

Add lodash.isequalwith to devDependencies #408

Merged

dependabot bot mentioned this pull request Mar 16, 2021

Bump connected-react-router from 6.8.0 to 6.9.1 webkom/lego-webapp#2139

Merged

apiiro-snyk mentioned this pull request Jul 2, 2024

[Snyk] Upgrade connected-react-router from 6.4.0 to 6.9.3 apiiro-snyk/react-boilerplate#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix location state (#394) #399

Fix location state (#394) #399

BlazPocrnja commented Mar 9, 2020

supasate Mar 14, 2020

supasate commented Mar 14, 2020

BlazPocrnja commented Mar 16, 2020 •

edited

Loading

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate Mar 17, 2020

supasate left a comment •

edited

Loading

BlazPocrnja commented Mar 21, 2020 •

edited

Loading

supasate commented Mar 22, 2020

supasate commented Mar 23, 2020

BlazPocrnja commented Mar 23, 2020

		onLocationChanged: PropTypes.func.isRequired,
		stateCompareFunction: PropTypes.func,

	return a === undefined \|\| (a.foo === "baz" && b.foo === 'bar') ? true : false
	return (a.foo === 'baz' && b.foo === 'bar') ? true : false

Fix location state (#394) #399

Fix location state (#394) #399

Conversation

BlazPocrnja commented Mar 9, 2020

Choose a reason for hiding this comment

supasate commented Mar 14, 2020

BlazPocrnja commented Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supasate left a comment • edited Loading

Choose a reason for hiding this comment

BlazPocrnja commented Mar 21, 2020 • edited Loading

supasate commented Mar 22, 2020

supasate commented Mar 23, 2020

BlazPocrnja commented Mar 23, 2020

BlazPocrnja commented Mar 16, 2020 •

edited

Loading

supasate left a comment •

edited

Loading

BlazPocrnja commented Mar 21, 2020 •

edited

Loading