Skip to content
This repository was archived by the owner on Oct 26, 2018. It is now read-only.

Sync: compare locations by structure #590

Closed

Conversation

smoogly
Copy link

@smoogly smoogly commented Jul 17, 2017

On first load history.transitionTo is called
even if state in store matches current history state
when selectLocationState doesn't preserve object reference
(for example using Immutable.fromJS(...).toJS(...)).

On first load `history.transitionTo` is called
even if state in store matches current history state
when `selectLocationState` doesn't preserve object reference
(for example using `Immutable.fromJS(...).toJS(...)`).
@timdorr
Copy link
Member

timdorr commented Jul 17, 2017

Unfortunately, this isn't what we want to test on. We want to ensure we're dealing with the same location instance. Also, this would cause issues with state being set to something non-serializable.

@timdorr timdorr closed this Jul 17, 2017
@smoogly
Copy link
Author

smoogly commented Jul 17, 2017

My thinking behind this is: selectLocationState is user-supplied method. There's no guarantee it's going to preserve references.

Which is exactly the case in this project, for example: https://github.com/FullstackTypeScript/redux-bootstrap/blob/master/src/index.tsx#L69
(value is passed through Immutable.js; see this pen: https://codepen.io/anon/pen/dREXbX?editors=0010)

When initialLocation isn't equal to locationInStore by reference, extra history transition happens on first load and pollutes browser history (back button stops working properly).

So, may I ask you to clarify: is it the fix incorrect or the problem should never be fixed in its entirety?

@smoogly
Copy link
Author

smoogly commented Jul 18, 2017

@timdorr, do you think being able to provide comparison function (which would default to reference comparison) is ok?
This way if selectLocationState for some reason doesn't preserve references, it will be possible to specify custom comparison to resolve this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants