Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React-router v3 compatibility #102

Closed
wants to merge 3 commits into from
Closed

React-router v3 compatibility #102

wants to merge 3 commits into from

Conversation

unRob
Copy link

@unRob unRob commented Oct 4, 2016

This "fix" feels like a hack, but it works well to prevent the error Uncaught TypeError: Cannot read property 'key' of null when using this library with react-router v3. All tests that passed without the change still do.

This "fix" feels like a hack, but it works well to prevent the error `Uncaught TypeError: Cannot read property 'key' of null` when using this library with react-router v3. All tests that passed without the change still do.
@taion
Copy link
Owner

taion commented Oct 4, 2016

That's really not the point. The incompatibility with history v3 is because history v3 has different semantics around the initial location – listen doesn't call the listener immediately with the initial location. It's not about hiding getCurrentLocation, or I wouldn't have added that check to React Router in the first place.

@taion taion closed this Oct 4, 2016
@taion
Copy link
Owner

taion commented Oct 4, 2016

Among other things, if you want to do this correctly, you must not invoke the listeners with the initial location in listen.

There's no point in making this library compatible with both history v2 and history v3, either – they're incompatible peer dependencies at any rate.

@taion
Copy link
Owner

taion commented Oct 4, 2016

Okay, on closer reading, this is close enough that I'll re-open the PR, but this PR needs to actually follow history v3 semantics before it's mergeable, and it needs to not fail the coverage check as well.

@taion taion reopened this Oct 4, 2016
@unRob
Copy link
Author

unRob commented Oct 12, 2016

Sorry I haven't found the time to work on this, but I hope this weekend I'll jump back on it. Still a bit confused at what you mean by follow history v3 semantics, could you clarify?

@taion
Copy link
Owner

taion commented Oct 12, 2016

listen should not immediately call back with the current location

@taion
Copy link
Owner

taion commented Nov 4, 2016

#107 supersedes this by removing the history integration entirely. There's no need to handle any of this just to be compatible with React Router v3, though bugs in history relating to hash history will still prevent scroll management from working correctly in such cases.

@taion taion closed this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants