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

Restore path on Devtools reset #74

Closed
wants to merge 1 commit into from

Conversation

ellbee
Copy link
Member

@ellbee ellbee commented Dec 4, 2015

I created a pull request here for discussion on how to fix this problem instead of cluttering up #73. This is my current best effort. I tried the approach of setting initialState to be null (or false etc.) initially and then filling it in when history.listen callback fired, but it caused a bunch of errors later on in other functions that were expecting the normal keys.

@jlongster
Copy link
Member

Why not just overwrite initialState entirely? I think this makes it more confusing than just resetting the whole thing.

@ellbee
Copy link
Member Author

ellbee commented Dec 6, 2015

Yeah, probably. The idea was to give an indication that something slightly weird is going on with the path in the initialState, rather than just overwriting it in a different part of the code.

I'll close this out. The solution for this was merged has a slight problem, but I'll open an issue for that.

@ellbee ellbee closed this Dec 6, 2015
@jlongster
Copy link
Member

In general though I like changing (not mutating, just resetting a var) the initialState more than how we currently do it. Currently in the store subscriber we check if we are using the initialState and it so, grab the first route loaded from history.listen. This causes inconsistent state at the beginning though; the state in the app state will not reflect the current route.

@ellbee ellbee deleted the restore_path_on_reset branch December 6, 2015 18:14
@jlongster
Copy link
Member

I actually like how this is headed though more than how we currently do it. I'd rather overwrite initialState instead of magically using a different state when initialState is passed in store.subscribe.

@ellbee
Copy link
Member Author

ellbee commented Dec 6, 2015

Is this along the lines of what you are thinking? #73 (comment)

@jlongster
Copy link
Member

Close, but I'd rather not depend on state in initialState to see if we should reset it or not. I might just make another local variable to track if it's the first load.

@ellbee
Copy link
Member Author

ellbee commented Dec 6, 2015

Ah, I see. That sounds good to me.

@jlongster
Copy link
Member

See this now: #83

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