Remove misleading example from upgrade guide

[billyjanitsch]

Per discussion in #2991, the second example is a fragile pattern. As @taion mentioned:

I don't think we really guarantee that you can just split out push like that, instead of calling it via router.push.

browserHistory is undefined in non-browser environments, so accessing browserHistory.push throws. It also relies on using the same history passed to Router, which may (conditionally) change.

Let me know if you'd prefer adding a warning over removing the example entirely.

[timdorr]

I'd say that example encourages bad patterns, so it should go completely. Thanks!

[taion]

Wait, no, this example is not broken for the same reason. The problem is something like:

<button onClick={browserHistory.goBack}>

This is fine, net of issues with binding handlers in render:

<button onClick={() => browserHistory.goBack()}>

[billyjanitsch]

<button onClick={() => browserHistory.goBack()}>

This doesn't break during a render test, but it does when you test the onClick, which is presumably something you'd want to do, no?

Also, the point wasn't whether this example is broken as-is -- just that it encourages a fragile pattern. The testing example is only a symptom.

[taion]

No, but if you're testing the click behavior, presumably you can take whatever steps are necessary to make this work appropriately. It's not my favorite pattern, but the one documented in the example is not as fragile as the one from your issue.

[agundermann]

I'd say that example encourages bad patterns, so it should go completely. Thanks!

👍

[taion]

Fine.

[ryanflorence]

???

There is only one url, there is nothing wrong with singleton histories. They aren't used in server rendering.

[ryanflorence]

Like, this is one of the major features of 2.0, singleton histories. Are people just freaking out by the word "singleton" or is there a real reason to discourage using them? Why even have them if we're going to discourage using them?

[taion]

I'd be happy with bringing this back, especially in light of discussion in reactjs/react-router-redux#257.

I'd just say that this pattern can be a bit dangerous on the server, and that in a component you "should" probably prefer context in general.

[timdorr]

Yeah, a better example would be using it outside of React entirely.

[ryanflorence]

I'd just say that this pattern can be a bit dangerous on the server, and that in a component you "should" probably prefer context in general.

You don't use them on the server.

[ryanflorence]

on the server all redirects are going to happen in route hooks, maybe that's the documentation that needs to happen.

[billyjanitsch]

@ryanflorence Take a look at discussion in #2991. It's not that they're used intentionally by the server, but that since their contracts differ in non-browser envs, they have potential to break the server/tests.

[taion]

If you're using something like async-props (or react-resolver), that can be hard. I think this is a fine pattern, but it runs a little bit of risk for being tricky for isomorphic rendering.

I don't think this is a good response to #2991, though – the example given in the docs here would not have issues when rendered on the server.

[ryanflorence]

As you say in #2991

since browserHistory is undefined [on the server]

Exactly as designed. There is no such thing as a browser history on the server.

<button onClick={() => browserHistory.push()}/>

What's wrong with that?

[taion]

@ryanflorence

Beat you to it: #2992 (comment) 😛

On balance, I think this usage is probably fine. I'm not going to use this syntax, but the cases where it causes problems are ones that novice users are unlikely to hit anyway.

[ryanflorence]

When you are server rendering, novice or pro, you must learn what stuff runs on the server and what runs on the client. didMount client. willMount server. Etc. You can't do willMount(){ someDomThing} anymore than <button onClick={browserHistory.push}/>.

There is no such thing as a browser history on the server. It should not be some object w/ a bunch of empty functions to allow naive button click handlers.

[ryanflorence]

We definitely need to have a doc that talks more about server rendering and gotchas like this.

[taion]

What I mean in the server rendering context is that if you call e.g. this.context.router.replace in componentWillMount, you'll be okay – if you do the same with browserHistory.replace, you won't be.

But this even is a more advanced usage, since normally (but not always) this can be dealt with in the onEnter hook.

Should we just revert this change for now?

[billyjanitsch]

<button onClick={() => browserHistory.push()}/>

What's wrong with that?

Suppose you have:

const MyButton = props => (
  <button onClick={() => {props.doThing(); browserHistory.push('...')}} />
)

Testing the callback will throw:

import {it} from 'mocha'
import {spy} from 'sinon'
import {shallow} from 'enzyme'
import {assert} from 'chai'

it('fires a callback when clicked', () => {
  const callback = spy()
  const button = shallow(<MyButton doThing={callback} />)
  button.simulate('click')
  assert.ok(callback.calledOnce)
})

I'm not suggesting that you should be doing this; what I took from #2991 is that it's an anti-pattern.

It should not be some object w/ a bunch of empty functions to allow naive button click handlers.

Right, but then why have an example in the docs with such a naive handler? I submitted this PR because I was led toward the pattern by that example. I also offered to add a warning instead of removing the example.

[timdorr]

I just want to avoid people doing silly things like browserHistory.push inside of an onEnter hook. (I've seen it attempted...) browserHistory usage should be thoughtful, that's all.

Maybe the example should be of an actual lifecycle function instead of something opaque.

[ryanflorence]

Ah ha!

Maybe we do export a warningHistory (ha!) with methods that all say "yo, this is being called in a non dom environment. if you're in a route hook, use the redirect arg in this function, if you're in willMount, use a route hook instead, otherwise, please open an issue so we know what the heck you're trying to do here!"

[taion]

That's a pretty contrived example that literally only comes up if you're using Enzyme, since React doesn't natively support simulating events on shallow rendered components. If you're running in Karma, browserHistory will be defined. That example really is not sufficiently motivating.

I definitely don't agree that the example led you to the pattern that you chose – I think the onClick={() => browserHistory.push(whatever)} pattern is much more idiomatic.

[taion]

@ryanflorence With e.g. async-props, can you access the async data in onEnter at all? If not, it'd seem like componentWillMount would be the best place to do a redirect.

[billyjanitsch]

@taion

That's a pretty contrived example that literally only comes up if you're using Enzyme

Enzyme is the wrapper I happen to use, but it seems like you'd want to test component methods regardless of framework, and folks are increasingly moving toward non-browser unit testing. Perhaps I'm wrong, but I don't think this type of situation is that uncommon.

I definitely don't agree that the example led you to the pattern that you chose

I saw the example, wrote some code that errored, thought "oh! it makes sense that this breaks, but I wonder why they suggest doing this kind of thing?" and opened #2991. I didn't contrive the example; it was my experience as a user encountering the docs.

I recognize the difference between the two examples, but I think it's an awfully large gotcha to go unexplained. I'm surprised by the hostility given that I opened this PR with the intention of improving the docs and offered to revise it with an explanatory warning instead.

@timdorr

Maybe the example should be of an actual lifecycle function instead of something opaque.

That sounds great -- perhaps also with a comment explaining the nuance. Happy to submit another PR.

[timdorr]

Probably submitted that right as you started writing. Check #3027 😄