Add warning to link.js

[lucasgodshalk]

I foolishly moved my Link component outside of the Router. I managed to figure it out pretty quickly but the error wasn't terribly helpful. See #3225. Thoughts?

[lucasgodshalk]

Would it be better to place this warning in the render method?

[taion]

Move this to the top of onClick and make it an invariant, something like

invariant(
  this.context.router,
  '<Link>s rendered outside of a router context cannot handle clicks.'
)

[taion]

Not in render because it would spam tests.

[taion]

Remove trailing whitespace here

[taion]

LGTM aside from that whitespace nit.

[lucasgodshalk]

Done.

[taion]

Thanks.

[timdorr]

I would move this to a lifecycle hook (probably componentWillMount) so developers can be alerted to mis-use earlier. I could see a Link not getting clicked on in testing and then going to production broken.

[taion]

We can't – the idea is that for unit tests, we let the link be rendered without a router, so that people can test non-navigation-related aspects of their page components that use <Link>s without having to set up a router context.

[lucasgodshalk]

Is there a code flag for unit tests?

[taion]

There's NODE_ENV, but it's uncommon for people to set it.

[taion]

What do we think here? I think putting the invariant in onClick lets us give users a helpful warning when they try to use a <Link> outside the router context. I don't think we can make them not render outside the router context, though.

[timdorr]

Well, given the problems with putting it on a normal lifecycle method, I guess that's out. I suppose having it on the click handler is better than nothing, but it still feels like we're not going to catch as many problems as we could.

[taion]

A while ago I suggested we should make router object required unless NODE_ENV === 'testing'. But nobody uses NODE_ENV === 'testing'. I mean... do you?

[taion]

Actually, that would be sort of counterproductive, since it means there'd be bugs in production that wouldn't get caught by tests. I dunno – it's hard.

[timdorr]

Yeah, unfortunately this isn't Rails where RAILS_ENV is de facto standard. And worse still, NODE_ENV doesn't exist in the browser context, so headless browser testing would break all of that anyways.

[taion]

Released with #3571 as v2.5.1.

[jebeck]

Any suggestions for unit testing now? I was using sinon to spy on onClick functions and simulating clicks with Enzyme. The use case is a bit odd perhaps - passing in an onClick function and want to prove that the click function is called/not called depending on whether a hidden bool passed in is true or false (this is on a slide-out nav panel for use on mobile). I can catch the errors to not have them fail the tests, but they obscure my spying on the onClick...

[lucasgodshalk]

I could move the warning down to below where we call an external onClick. When you have your own onClick, the Link doesn't requires access to the router context.

[taion]

Only if you preventDefault...

Can you just inject a fake router context? And can you open an issue for this?