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

Support history v3 #3647

Merged
merged 1 commit into from
Jul 19, 2016
Merged

Support history v3 #3647

merged 1 commit into from
Jul 19, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Jul 19, 2016

This code isn't going to win any beauty awards. I'd like to clean this up structurally before v3.0.0 final if I have time, but it's not going to involve any major API changes (just internal refactoring).

I figure it's okay to take out the invariant on history.getCurrentLocation because anybody attempting to use a v2 history will just hit an exception when history.getCurrentLocation fails due to not existing. If it comes up, it'd be easy enough to add back the inverse of the removed invariant to assert a non-v3 history.

// <Router> and attaches another history listener.
if (unlisten) {
unlisten()
}
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see this go!

@timdorr
Copy link
Member

timdorr commented Jul 19, 2016

I figure it's okay to take out the invariant on history.getCurrentLocation because anybody attempting to use a v2 history will just hit an exception when history.getCurrentLocation fails due to not existing.

I think it would be better to add back the invariant than let the engine handle it. It will probably not be super-helpful to people unless they're familiar with the API change between v2 and v3, in which case they probably aren't going to run into the issue anyways.

@taion
Copy link
Contributor Author

taion commented Jul 19, 2016

Sure. Added back the invariant.

@timdorr
Copy link
Member

timdorr commented Jul 19, 2016

👍

@taion
Copy link
Contributor Author

taion commented Jul 19, 2016

Is that an LGTM? I think we should cut this as an alpha.2 while we're still working through a few last things on history v3.

@timdorr
Copy link
Member

timdorr commented Jul 19, 2016

Yep, go nuts.

@timdorr timdorr merged commit 9a2b026 into next Jul 19, 2016
@timdorr timdorr deleted the history-v3 branch July 19, 2016 15:07
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
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