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

[3.0] Remove deprecations. #3340

Merged
merged 12 commits into from
Apr 21, 2016
Merged

[3.0] Remove deprecations. #3340

merged 12 commits into from
Apr 21, 2016

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Apr 18, 2016

See #3336.

@timdorr timdorr changed the title [3.0] Remove context.history/location and History mixin [3.0] Remove some deprecations. Apr 18, 2016
@timdorr
Copy link
Member Author

timdorr commented Apr 18, 2016

Removing many of the deprecations involves fairly interdependent code, so this will be just one single PR for a bunch (all?) of the deprecations.

warning(false, '`Router` no longer defaults the history prop to hash history. Please use the `hashHistory` singleton instead. http://tiny.cc/router-defaulthistory')
createHistory = createHashHistory
}
history = { ...history, ...transitionManager }
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need the "routing history" any more. This function shouldn't return history at all.

@timdorr timdorr force-pushed the remove-context-history-location branch from 15016f2 to ee3d058 Compare April 18, 2016 15:05
@taion
Copy link
Contributor

taion commented Apr 18, 2016

@timdorr

Do you mind if we leave this PR open until we've taken everything out, so that we still have access to the PR comments?

BTW, the idea w/history is that, once we deprecate everything, it should only show up as a prop to <Router> (and input to match), and nowhere else – nothing downstream gets a history any more.

@timdorr
Copy link
Member Author

timdorr commented Apr 18, 2016

Yeah, no rush on this.

@taion taion added this to the next-3.0.0 milestone Apr 18, 2016
@timdorr timdorr force-pushed the remove-context-history-location branch from 9aedfec to 0dca89a Compare April 18, 2016 15:32
} else {
nextStateWithLocation = { ...nextState, ...location }
getComponent.call(route, nextState, callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this shouldn't be in an else. A number of linting rules prohibit "pointless" else clauses per https://github.com/airbnb/javascript/blob/a9db06e1ddbefe3342a61b649c104420d82ba37d/packages/eslint-config-airbnb/rules/best-practices.js#L39. Just put this outside the if block as in #3348.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see what you did – the idea with the !getComponent check above and the reason I just bail out there is because it's intended as a guard clause. Otherwise we should probably invert the order of the branches.

@timdorr timdorr force-pushed the remove-context-history-location branch from 5e49a1e to eab6f08 Compare April 18, 2016 20:35
@timdorr
Copy link
Member Author

timdorr commented Apr 18, 2016

Rebased against next, which was updated against master.

Anything left? I think this is good to go.

@@ -226,15 +210,6 @@ export default function createTransitionManager(history, routes) {
if (history.listenBeforeUnload)
unlistenBeforeUnload = history.listenBeforeUnload(beforeUnloadHook)
}
} else {
if (hooks.indexOf(hook) === -1) {
Copy link
Contributor

@taion taion Apr 18, 2016

Choose a reason for hiding this comment

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

This is not quite right – RouteHooks[routeID] should actually no longer be an array at all, and this else branch should instead just overwrite the value.

So this whole thing should be something like:

const thereWereNoRouteHooks = !hasAnyProperties(RouteHooks)

const routeID = getRouteID(route)
RouteHooks[routeID] = hook

if (thereWereNoRouteHooks) {
  // ...

Copy link
Member Author

@timdorr timdorr Apr 18, 2016

Choose a reason for hiding this comment

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

Ah crap, I was racing through this to get back to work. I'll have to look at it tonight.

@timdorr timdorr changed the title [3.0] Remove some deprecations. [3.0] Remove deprecations. Apr 18, 2016
@timdorr
Copy link
Member Author

timdorr commented Apr 20, 2016

Fixed up listenBeforeLeavingRoute. Let me know if there's anything else.

I'll want to merge up next and then rebase this before merging.

@taion
Copy link
Contributor

taion commented Apr 20, 2016

https://github.com/reactjs/react-router/blob/d3be55cc6b80506456f3d91e53b52046cb480670/modules/createTransitionManager.js#L104-L107 needs to be updated, something like

const hooks = routes
  .map(route => RouteHooks[getRouteID(route)])
  .filter(hook => hook)


if (!hooks) {
let thereWereNoRouteHooks = !hasAnyProperties(RouteHooks)
RouteHooks[routeID] = [ hook ]
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the array here at all, unless I'm missing something.

@timdorr
Copy link
Member Author

timdorr commented Apr 20, 2016

Ah, that's what I was missing!

@timdorr timdorr force-pushed the remove-context-history-location branch from d3be55c to 56b0be0 Compare April 20, 2016 04:08
return hooks
}, [])
return routes
.map(route => RouteHooks[getRouteID(route)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, this should probably be getRouteID(route, false), actually. No reason to create route IDs here. The undefined will safely avoid colliding with any actual keys under RouteHooks.

@timdorr timdorr force-pushed the remove-context-history-location branch from 3a30d28 to 1a5666e Compare April 21, 2016 05:30
@timdorr
Copy link
Member Author

timdorr commented Apr 21, 2016

OK, updated next against master and rebased this branch. Should be all good to go.

@gaearon
Copy link
Contributor

gaearon commented May 5, 2016

Sorry if this is unrelated—shouldn’t https://github.com/reactjs/react-router/blob/next/modules/RoutingContext.js also go?

@timdorr
Copy link
Member Author

timdorr commented May 5, 2016

Good catch! Yeah, I'll drop that one too.

@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.

4 participants