Redirect on props update (#5003)

[alexilyaev]

Fixes #5003

Is it ok I've added is-equal dependency? It's used by expect already.

I needed it because checking if we are redirecting to the same route is not so trivial.
to can be a String or an Object, and the Object variant can have pathname, search and state, each of which could be a valid new route to redirect to.

e.g. I might want to redirect to my own route with a different search string or state data.

Other PR Questions:

• I'd really like the warning to be better, telling the user where that last originated or something that will help him realize where the issue is, any ideas?
• How do I test if the usage of warning?
  In this case if we try to redirect to the same route we call warning to notify the user.

[timdorr]

Please don't add to the gitignore file. This should be in your global .gitignore file for your machine.

[alexilyaev]

Good point, removed.

[timdorr]

pathname and search are simple strings, so that's easy to check. I don't even know if state is worth checking, since it's a hidden property anyways.

There is a warning test at the bottom of #5151

[alexilyaev]

@timdorr Update:

• Ditched is-equal and wrote a basic comparison without state, is-equal didn't catch an edge case any way.
• I've noticed to is not a required prop, does that make sense? What's the use case for a Redirect without to?
• The tests in Issue #5114 warn about history prop in Router-derived components #5151 are for console, which is a global object, if we want to test warning, which is an imported module inside the Redirect module, we'll have to use proxyquire or alike to stub it.
  Let me know if you'd like me to do that, for now I've added a test that works around that.

[timdorr]

Two ideas:

1. Use value-equal, which is fairly lightweight.
2. Use history/LocationUtils -> locationsAreEqual. It requires that it's an object, but you can also use createLocation to coerce it into an object.

I don't know why to isn't required. We're not hardcore PropType-ers around here.

It's fine to test console. You don't need to stub the library, just the effects of that library. If we want to switch to a "better" library at some point, we can do so without breaking tests.

[alexilyaev]

@timdorr value-equal worked, so did locationsAreEqual, so I picked it to not add a dependency.
Spying on console.error worked as well.

The warning itself has a stack trace, but you can't find the component that included the bad <Redirect>. This would be a different ticket though.

I think I'm done.

[alexilyaev]

@timdorr Merge it while it's hot (and passing build/tests) ;-)

[timdorr]

LGTM. Needs a second, though. (Only @ryanflorence or @mjackson can do a solo merge)

[mjackson]

I'm tempted to just put an invariant inside <Redirect>'s componentDidUpdate. It's not supposed to persist across multiple renders!

[timdorr]

It's not that it's necessarily persisting the same exact <Redirect> usage in a user's code. It's that React is re-using an instance, as it does when the component tree shape does not change. So this is perfectly reasonable code to write:

render() {
  return (
    !loggedIn ? <Redirect to="/login"/> : 
    !admin ? <Redirect to="/denied"/> : 
    <Admin/>
  )
}

You've got two <Redirect>s from your perspective, but if you move between them, React re-uses the instance and it does nothing. No one is trying to re-use a <Redirect>, React is. It's a common problem to run into.

[alexilyaev]

Fixed conflict, but the build was canceled, no apparent reason.
Can we rebuild it?

[timdorr]

It auto-cancels if another build comes in on top of it. But you should rebase against master because @mjackson just fixed all the testing and building infra.

[alexilyaev]

@timdorr Had some issues, but managed to do it eventually.
In Jest, we don't need to restore spies anymore?
Also, do I need to squash the commits or something? Got a ref for that?

[pshrmn]

@alexilyaev Squashing yourself isn't completely necessary since GitHub allows the maintainers to do it when merging, but I generally like to do so anyways (especially if something I do in one commit gets reverted in another). For reference, this article goes over the details very nicely: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request.

[alexilyaev]

Guys, please merge this.

[timdorr]

Needs another 👍 from someone.

[pshrmn]

I would prefer if this was two statements.

[alexilyaev]

@pshrmn Can you share exactly what you'd expect?
I think interpolating inside the template string didn't break well.
Unless you mean to ditch the %s%s and do a string concatenation?

[pshrmn]

The current code is essentially the same as return console.error(...). While that is legal, I think that it would be better to separate the call expression and return statement.

if (...) {
  warning(...)
  return
}

[pshrmn]

I think that this should use createLocation because it wouldn't work for <Redirect to='/somewhere?test=ing' />.

[alexilyaev]

@pshrmn Turns out createLocation was exactly what I needed, thanks!
Added a test for this use case.

[alexilyaev]

@pshrmn Done with requested changes.
Now using createLocation and added a test for the query param use case.

[timdorr]

In it goes. It'll be in the next minor release. Lots of goodies lined up, so hopefully that will be soon.