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

Don't navigate immediately on boot. #81

Merged
merged 1 commit into from
Dec 6, 2015
Merged

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Dec 6, 2015

Had an issue in my app, in combination with react-fetcher, where redux-simple-router would create a history navigation on load, even though there's no reason to immediately navigate right on boot (the url is already in the URL bar).

Was this behavior intentional? Changing this line fixed the issue for me and I no longer have a redundant UPDATE_PATH on boot and an additional fetch.

If it helps, I'm pairing this with populating state.routing.path on the server side using req.url, so the routing path is correctly pre-populated on boot.

@jlongster
Copy link
Member

The problem is that there's no way to get the "current location", so initialState has the wrong URL (it just defaults to /). It will dispatch one action to update the store with the current URL.

With your changes here, the app state will not have the correct route for the initial route.

@jlongster
Copy link
Member

We can get around this by creating a different action type, I suppose, something like INIT_PATH.

@STRML
Copy link
Contributor Author

STRML commented Dec 6, 2015

Actually, it seems that it works fine. I'm not preventing the state from updating (UPDATE_PATH still fires), it just prevents a useless history.pushState().

For example (PENDING, FULFILLED actions are from react-fetcher on the route component):

screen shot 2015-12-06 at 12 45 18 pm

@jlongster
Copy link
Member

Ah, I see what you mean. That makes sense. I'm actually making some changing in this area but I'll go ahead and merge this.

jlongster added a commit that referenced this pull request Dec 6, 2015
Don't navigate immediately on boot.
@jlongster jlongster merged commit 9941838 into reactjs:master Dec 6, 2015
@STRML
Copy link
Contributor Author

STRML commented Dec 6, 2015

Thanks!

@jlongster
Copy link
Member

I think this actually broke some tests. I need to integrate TravisCI on here.

I'll look into it.

@jlongster
Copy link
Member

Ok, so I need to back this out because it broke tests, most importantly these 2:

https://github.com/jlongster/redux-simple-router/blob/master/test/createTests.js#L436
https://github.com/jlongster/redux-simple-router/blob/master/test/createTests.js#L505

Please wait a day or two because I'm going to land some changes with this code, and it might fix your issue.

@STRML
Copy link
Contributor Author

STRML commented Dec 6, 2015

No worries. Thanks.

@jlongster
Copy link
Member

See #83 now.

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