-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Warning: You cannot PUSH the same path using hash history #4467
Comments
Seems like a weird warning. You can't really tell the user not to click links, and it'd be really cumbersome to make the link not link to anywhere when its active. Unless @mjackson has some reason I'm unaware of, we'd happily merge a PR that removes the warning and ship a release. |
The warning is there just to let you know that when you're using hash history, you can't actually PUSH the same path; the browser won't add anything to the history stack. But you should only get this warning in development. If you generate your production build correctly (using |
Can we just leave that to docs and not a warning? |
If the warning stays, having a console.warn would be helpful. I thought I was doing something wrong, but this isn't really an error. |
@mjackson any plans on changing it ? |
@ryanflorence @mjackson I'm pretty convinced this warning should go - even if it is development only, it adds a ton of noise for developers who want to distill down to real errors, and in some cases it's an annoying implementation hoop to jump through in order to avoid a redundant push. Since the history library is set up to ignore redundant pushes there's not really a need to issue a warning. If you ask me. 🙂 |
It really should stay. It's very easy to miss that you're trying to push the same path and makes it hard to track down the error. Clicking something and nothing happening is a terrible and frustrating DX because there is nothing to trace. |
^ fair point I hadn't considered. |
I agree with @sloansparger, If it's a warning (which it says it is), then it should use |
An error gives the message this will break the app, which it will not necessarily. The message is for debugging purposes. |
Hey - worth noting that if this were to change it would be a change to the history package, not react-router. History's warning messages all rely on the Facebook-inspired warning package. Seems like this should be a discussion of whether to use that package in the history module, and the canonical reason for keeping the red error message for warnings is that they offer stack traces, whereas |
@benwiley4000 The stack trace argument is great! Seems the history module already has an issue filed: remix-run/history#488 In order to get a stack trace, all you need to do is wrap the warning with a
I'm not quite sure why you'd need to use a package for something like this anyway. But I'd imagine there are reasons. |
Wow @arjunmehta - stealing that trick for the future! |
you can really use Here is example on how you render multiple links while avoiding this warning if it's too annoying const renderLinks = (currentPath) => {
return <ul>
{
routes
.map(({ path }, i) =>
<li key={i}>
<Link
to={`/${path}`}
replace={path === currentPath}>
{title}
</Link>
</li>
)
}
</ul>
}
|
This is very similar to #2431, where I'm getting this warning.
Luckily, remix-run/history#43 seems to fix it in the general case, but I'm getting this warning in a very specific instance.
With a basic application with:
The first time I load the application at http://domain-name/#/ and click on a Link to '/', I get this error. Afterwards, I never see it again.
Is this intended or did I set something up incorrectly?
The text was updated successfully, but these errors were encountered: