-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[Bug]: useLocation inside <Routes location={location}> doesn't return given location #8470
Comments
let location = React.useMemo(() => {
let trailingPathname = stripBasename(pathname, basename);
if (trailingPathname == null) {
return null;
}
return {
pathname: trailingPathname,
search,
hash,
state,
key
};
}, [basename, pathname, search, hash, state, key]); The |
@liuhanqu Yup you're right. But the issue here seems to be that there's only one location in context. I expect this behavior: |
I had to use a temporary workaround for now because this is blocking my huge release!
export const Location = createContext();
export default function Routes() {
const location = useLocation();
let background = null;
if (location?.state?.modal && location?.state?.background) {
// "layered" logic might work differently in your app but
// when modal is opened, I've got the following state:
// {modal: true, background: previousLocation}
background = location.state.background;
}
return (
<>
<Location.Provider value={background || location}> {/* this right here officer */}
<Routes location={background || location}>
{/* normal/background routes */}
</Routes>
</Location.Provider>
{background &&
<Routes>
{/* overlay/modal routes */}
</Routes>
}
</>
);
}
export default function useLocation() {
const scopedLocation = useContext(Location);
const globalLocation = useReactRouterLocation(); // "real" useLocation
const modalIsOpen = scopedLocation?.pathname !== globalLocation?.pathname;
const thisHookIsCalledFromModal = modalIsOpen && !scopedLocation && globalLocation?.state?.modal;
return (thisHookIsCalledFromModal ? globalLocation : scopedLocation);
} Dadaa! Now |
Gah! This bug just cost me a few hours... Thanks @gitcatrat for reporting and giving a workaround! |
Unfortunately, I'm using |
This issue also cost me a few hours! I'm also using
While navigating from page A to page B, both pages render with the current This is the last thing I have to do to complete upgrade to ReactRouter v6. |
In the source code we can see that indeed import { Action } from 'history';
import { UNSAFE_LocationContext as LocationContext } from 'react-router-dom'
// wherever you need the stored location, or anywhere higher in the tree
<LocationContext.Provider value={{ location: storedLocation, navigationType: Action.Pop }}>
{...}
</LocationContext.Provider> I expected #8008 to mention this issue as well. |
Well, this sucks. This breaks functionality for anyone using the background modal technique that dates all the way back to React Router v1. The modal example in the official documentation is also affected by this. As mentioned above, the location object given as a prop to Routes and then passed on to useRoutes, is not used. Only the pathname property of the passed location is used. The workaround using a custom location context feels a bit hackish and requires making changes to any components that use location for logic (or undesired things will happen behind the modal). I considered reverting to v5 but for now I have simply reset my isModal state to false before rendering my routes, which causes a navigation instead of opening the modal. Michael or Tim @mjackson @timdorr, please could you share your opinion on this issue? |
You don't need to make changes to all components that use location for logic. You only need it around the modals. A single context provider would work. (That said, it feels very hacky) |
@SleeplessByte I tried your workaround but it doesn't seem to work with components that use other hooks such as useParams(). The path matching seems to be broken. It works when I create my own LocationContext and use it to pass down my stored location: export const LocationContext = createContext(); <LocationContext.Provider value={isModal ? prevLocation : location}>
...
</LocationContext.Provider> Then I have to do this across my whole codebase: - import { useLocation } from 'react-router-dom';
+ import { LocationContext } from 'myApp';
- const location = useLocation();
+ const location = useContext(LocationContext); |
I think the provider may not be at the "right" location then. I use this workaround in production. Using your own context (and hook) definitely should work 😁 |
@SleeplessByte You were right, the scope of my provider was too large and so I was passing the wrong location to my modal (God, this is confusing!!). Your workaround is a nice solution. Thanks for your help! The only change, instead of importing Action from history, I'm getting the navigation type from the hook: const navigationType = useNavigationType(); |
Yeah. That should work. Instead of hardcoding it, that should actually retrieve the value dynamically. |
any plans to solve this ? |
It would be great to know if the maintainers agree that it's a bug and that |
I'm going to defer to @mjackson and @ryanflorence here (as they've got the historical insight) but this doesn't feel like a bug to me. The If we made the change in #9094, wouldn't that just introduce the opposite problem in that contextual components could never actually access the true global location anymore? Seems like we're trading one problem for another. In the example provided, couldn't components that need to use a contextual location just do something like the following: const location = useLocation();
const contextualLocation = location.state?.background || location; That now leaves the decision of which |
It feels very weird to have to:
I would be completely okay with keeping the workaround I listed, but I do think something needs to be done in order to support #8008. |
This is particularly useful for when you have a modal that has its own URL. For example: https://v5.reactrouter.com/web/example/modal-gallery
The idea is that the components that are receiving the "background" location work as usual. They are not aware of the concept of background/foreground location. To be honest I have never come across a use case where the background location would need to access the "global" location.
The problem is that the components where we need this behavior are
|
@brophdawg11 I can change #9094 to add the global location to the LocationContext and add a hook to fetch global location if someone would want that. That way the current behavior could still be accessed if needed. |
I totally agree with @zhouzi and @johnpangalos. @brophdawg11 my intuition about I didn’t consider the possiblility that the current behavior might possibly be intentional at all. I went in here with a very clear feeling of “ah, good thing this bug is finally being addressed”. As you can see from @zhouzi’s comment, merging #9094 would allow library authors to rely on the ability to truly override the hook’s return value. This alone is an indicator of good design. |
The gallery example uses a different set of routes for the modal and for the background, I guess the fact that you can use different routes within the same top-level router alone warrants |
This issue just caused me several hours of debugging. It is a major breaking change that is never mentioned in the documentation. I agree with others that the most intuitive behavior is that const globalLocation = useLocation();
const location = globalLocation.state?.backgroundLocation || globalLocation; Any plans for fixing that? I think most use cases of The interface mentions: /**
* Returns the current location object, which represents the current URL in web
* browsers.
*
* Note: If you're using this it may mean you're doing some of your own
* "routing" in your app, and we'd like to know what your use case is. We may
* be able to provide something higher-level to better suit your needs.
*
* @see https://reactrouter.com/docs/en/v6/api#uselocation
*/
export declare function useLocation(): Location; I would argue that, in order to do "some of your own routing", you need the contextual location. |
Hey folks - I checked in with @ryanflorence and he confirmed this is a bug, so we should be able to get #9094 merged shortly for release with |
What version of React Router are you using?
6.1.1
Steps to Reproduce
Full reproduction can be found here, it's functionally similar to v5 modal gallery example:
https://codesandbox.io/s/heuristic-rhodes-508k5?file=/src/App.js
Steps to replicate:
useLocation
in Home still returns new locationExpected Behavior
useLocation
inside<Routes location={location}>
returns theprop
location.useLocation
inside<Routes>
returns the "correct" currently active location.Actual Behavior
Both
useLocation
return "correct" currently active location (as the one corresponding to URL bar).I.e issue seems to be that
useLocation
is not scoped, it only reads global location.The text was updated successfully, but these errors were encountered: