Skip to content
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

Potential useSearch bug not triggering updates when used with urql #393

Open
junwen-k opened this issue Dec 1, 2023 · 10 comments · May be fixed by #422
Open

Potential useSearch bug not triggering updates when used with urql #393

junwen-k opened this issue Dec 1, 2023 · 10 comments · May be fixed by #422
Labels
bug Something isn't working

Comments

@junwen-k
Copy link

junwen-k commented Dec 1, 2023

Greetings, I've been scratching my head over this issue for quite awhile.
Following #368, I've been using the useSearchParams hook and it has been working fine, until I started integrating with urql's useQuery.

Issue

useSearch does not rerender if it begins with query params and revisited. For example, if I land on the page with URL

http://localhost:5173/?tab=1

I cannot go back to original tab after switching to another tab. (URL has been updated to ?tab=1, but React does not rerender the components. You may see the video below:

Screen.Recording.2023-12-01.at.8.59.27.PM.mov

Reproduction Steps

  1. Clone https://github.com/junwen-k/react-router-wouter

     git clone https://github.com/junwen-k/react-router-wouter
     cd react-router-wouter
    
  2. Install dependencies

     npm install
    
  3. Start Server

     npm start
    
  4. Experiment with the page, with default ?tab=1 triggers the issue

Context

Suspect pushState event does not trigger, hence React does not rerenders the component.
React Router's version works perfectly, so I suspect this is something to do with wouter.

@junwen-k
Copy link
Author

junwen-k commented Dec 2, 2023

After playing around and some debugging, I notice that when I do

packages/wouter/src/use-browser-location.js

export const useSearch = ({ ssrSearch = "" } = {}) => {
  return useLocationProperty(
    () => location.search,
    () => ssrSearch
  );
};

Instead of

const currentSearch = () => location.search;

export const useSearch = ({ ssrSearch = "" } = {}) =>
  useLocationProperty(currentSearch, () => ssrSearch);

The issue seems to be fixed. Could this be a potential bug with how the library uses useSyncExternalStore?

Screen.Recording.2023-12-02.at.4.38.19.PM.mov

If this is really the cause of the bug, does that mean that usePathname and useHistoryState could potentially having the same exact issue as well?

export const useSearch = ({ ssrSearch = "" } = {}) =>
  useLocationProperty(
    () => location.search,
    () => ssrSearch
  );

// ...

export const usePathname = ({ ssrPath } = {}) =>
  useLocationProperty(
    () => location.pathname,
    ssrPath ? () => ssrPath : location.pathname
  );

// ...

export const useHistoryState = () =>
  useLocationProperty(
    () => history.state,
    () => null
  );

I am guessing, when we define

const currentSearch = () => location.search;

The value of currentSearch got "memorised" during initialisation, which causes this issue...

Any thoughts / comments are welcomed!

@molefrog molefrog added bug Something isn't working V3 Features that won't be released in v2, but planned for the next major release labels Dec 11, 2023
@molefrog
Copy link
Owner

@junwen-k Hi! My guess it that useSearchParams from RR isn't compatible with wouter and it requires a separate implementation which is wouter-specific. I have an idea, as an experiment, use this dummy hook instead:

const useSearchParams = () => {
  const [,navigate] = useLocation()
  const search = useSearch()
  const params = new URLSearchParams(window.location.search)

  const setSearchParams = (obj) => {
    navigate(`?tab=${obj.tab}`)
  } 

  return [params, setSearchParams]
}

@molefrog
Copy link
Owner

Also, take a look at #391

@junwen-k
Copy link
Author

@junwen-k Hi! My guess it that useSearchParams from RR isn't compatible with wouter and it requires a separate implementation which is wouter-specific. I have an idea, as an experiment, use this dummy hook instead:

const useSearchParams = () => {
  const [,navigate] = useLocation()
  const search = useSearch()
  const params = new URLSearchParams(window.location.search)

  const setSearchParams = (obj) => {
    navigate(`?tab=${obj.tab}`)
  } 

  return [params, setSearchParams]
}

Hi, thank you for replying. I've tested with the above snippet and the issue seems to persist. 👀
The behaviour is very buggy as you can see that sometimes it "works".

useSearchParams.ts

import { useLocation, useSearch } from "wouter";

export const useSearchParams = () => {
  const [, navigate] = useLocation();
  const search = useSearch();
  const params = new URLSearchParams(window.location.search);

  const setSearchParams = (obj) => {
    navigate(`?tab=${obj.tab}`);
  };

  return [params, setSearchParams];
};
Screen.Recording.2023-12-12.at.4.03.05.PM.mov

You can see that for a split second, Tab 3 works for a while, then it won't work again 🤷🏻‍♂️

(0:20: Refresh with ?tab=3 as default)
(0:22, 0:24: Tab 3 is selected and worked as expected?)

@molefrog
Copy link
Owner

Right, I was able to reproduce it locally. What bothers me is that React Router also doesn't re-render when the query string changes from the outside. Try running: history.pushState(null, "", "/?tab=3") and you will see that the tab isn't updated.

@junwen-k
Copy link
Author

junwen-k commented Dec 12, 2023

I notice that when I do

const subscribeToLocationUpdates = (callback) => {
  for (const event of events) {
    addEventListener(event, (...props) => {
      console.log("event", event, ...props);
      callback(props);
    });
  }
  return () => {
    for (const event of events) {
      removeEventListener(event, callback);
    }
  };
};

Console actually output:

event pushState Event {isTrusted: false, arguments: Arguments(3), type: 'pushState', target: Window, currentTarget: Window, …}

Which means pushState event is working. When I inline the arrow callback and it works as expected.

export const useSearch = ({ ssrSearch = "" } = {}) =>
  useLocationProperty(
    () => location.search, // Inlined
    () => ssrSearch
  );

May I know if this is not the right approach? I have the impression that somehow the value of location.search gets memorised or something.

I've also noticed that react-router discourage users to manipulate URLs using the history object directly.

⚠️ IMPORTANT
For illustration only, don't use window.history.pushState directly in React Router

Not sure if running history.pushState(null, "", "/?tab=3") not working for react-router was due to that.

@molefrog
Copy link
Owner

molefrog commented Dec 14, 2023

Not sure if running history.pushState(null, "", "/?tab=3") not working for react-router was due to that.

I did some digging: React Router uses its own format of state object to perform navigation. It is not designed to be compatible with external routers, and that is completely okay as it means that wouter doesn't break it. So I narrowed down the example to only include wouter and did two experiments:

  1. Instead of using useSearchParams I use useLocation (as well as useHashLocation) to extract the current tab number from the URL, e.g. /1
  2. Replaced that hook with a simple useState

Interestingly, 2) works well, while 1) is still broken. It proves that the issue is indeed in wouter, but it also tells us that it's not just the search string hook.

I'll keep looking

@molefrog
Copy link
Owner

Hey @junwen-k any update on this? I'm curious if you found out the issue.

@junwen-k
Copy link
Author

junwen-k commented Feb 27, 2024

Hey @junwen-k any update on this? I'm curious if you found out the issue.

Hi, thank you for following up. As mentioned earlier, I've actually found out a solution that worked on my end.

The changes that fixes the issue was:

- const currentSearch = () => location.search;
- 
export const useSearch = ({ ssrSearch = "" } = {}) =>
- useLocationProperty(currentSearch, () => ssrSearch);
+  useLocationProperty(
+    () => location.search,
+    () => ssrSearch
+  );

I am also wondering if we should change history as well:

- const currentHistoryState = () => history.state;
-
export const useHistoryState = () =>
-  useLocationProperty(currentHistoryState, () => null);
+  useLocationProperty(
+    () => history.state,
+    () => null
+  );

However as mentioned earlier, I am not sure if this is the right approach to the issue.

I have the impression that somehow the value of location.search gets memorised or something.

Currently as a workaround, I am using my upstream branch which you can take a look here. Note that this is tightly related to #391 as the useSearchParams hook won't really work without this fix.

@pReya
Copy link

pReya commented Apr 29, 2024

Suffering from the same bug. Would appreciate if the connected PR was merged soon.

@molefrog molefrog removed the V3 Features that won't be released in v2, but planned for the next major release label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants