Drop "cannot go" invariant in createMemoryHistory

[petrbrzek]

Is it important to throw an error if I cannot goBack? It seems to me that returning null should be just fine.

[taion]

Ref #89, and transitively remix-run/react-router#767 and remix-run/react-router#843.

I don't think this can be resolved by #36.

I don't have the time right now to fully read through those refs to understand why history.length isn't possible, but if that's not possible, then this might not be possible as well.

[petrbrzek]

Well, there is the canGo function. Can't we just expose it?
Or, do we really need the invariant call below? I think it's cool for dev mode but not for production.

invariant(
  canGo(n),
  'Cannot go(%s) there is not enough history',
  n
)

[taion]

Are you using createMemoryHistory?

[petrbrzek]

Yep.

[taion]

Ah! Okay. That's quite relevant!

[taion]

Can you just wrap the whole thing in try / catch then?

[petrbrzek]

I can, but it's kinda ugly.

[taion]

I'm on board with replacing the invariant with a warning. It'd be more consistent with what browser histories do (in doing nothing if you're already at the beginning of the history stack).

This is technically a breaking change, though, so we'd have to defer it for 2.x.

@rackt/routing, what do we think of dropping that invariant to make createMemoryHistory behave more like the others?

[taion]

I like your suggestion of dropping the invariant, BTW. Maybe a warning. I edited the title of the issue as appropriate.

[petrbrzek]

warning is okay. 👍

[mjackson]

The invariant is meant to help you find errors in your code. A warning would serve the same purpose, I guess. But if you're trying to e.g. go(-5) and you're only on the first page, you should probably not do that ;)

I'd like to expose canGo, but we can't actually do it in real browser histories, so the API wouldn't be consistent. See the issues @taion mentioned for the looooooong backstory.

[taion]

I'll look at replacing it with a warning. Gets you the best of both.

[taion]

Actually, @petrbrzek, do you want to put together a PR? I'm a bit busy the next few weeks, have a conference and a deadline coming up.

[petrbrzek]

@taion Sure. What about this?

if (!canGo(n)) {
  warning(
    false,
    'Cannot go(%s) there is not enough history',
    n
  )
  return null
}

[taion]

Just return - this function has no return value anyway.

[taion]

But otherwise that's exactly what I was thinking. I don't know what test coverage around that looks like though - not sure what we need there either.

[mjackson]

Shouldn't need a test. Simple replace with a warning is fine.

On Wed, Dec 2, 2015 at 8:02 PM Jimmy Jia notifications@github.com wrote:

But otherwise that's exactly what I was thinking. I don't know what test
coverage around that looks like though - not sure what we need there either.

—
Reply to this email directly or view it on GitHub
#165 (comment).

[taion]

👍

[tommoor]

if you're trying to e.g. go(-5) and you're only on the first page, you should probably not do that ;)

@mjackson how can we avoid this if the history doesn't expose a length or a canGo method? Geniunely asking - there seem to be 100 different issues related to this but no real solution.

[mjackson]

@tommoor What are you trying to do? As in, what feature or app are you trying to build? The reason I ask is because we're too far down in the weeds of this discussion when we start talking about API that we need but that we just can't provide.

[tommoor]

@mjackson - that's fair - I'm trying to provide back and forward buttons in our app that are disabled correctly when you wouldn't be able to navigate in the direction because you are at the start or end of the history.

[mjackson]

Maybe we could just expose canGo for memory histories, with the caveat that browser histories won't ever have it?

[tommoor]

That would certainly resolve it for us as we're using the memory history - it's an Electron app in this case. Is there a lot of work to achieve that?

[taion]

You'd get something like this with location.index and something like a history.getMaxIndex().

[mjackson]

Mmmmmm, yes. If we had location.index and history.length this would be trivial. Let's follow up in #334.