TanStack / router

🤖 Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
7.76k stars 564 forks source link

window.history methods cloned early when createBrowserHistory is called which can break other services e.g Google tag manager #2169

Closed GregoryCollett closed 2 weeks ago

GregoryCollett commented 3 weeks ago

Describe the bug

window.history methods e.g. pushState and replaceState are cloned early in the router creation meaning that other asynchronous services which may also make modifications to window.history methods do not work as expected. Given these services are asynchronous it is completely time based and can sometimes work if it manages to make its changed before the router is initialised.

For example. On my production page there is a GTM script, if the router happens to initialise before the GTM script has loaded and run its relevant ASYNC dependencies then the modifications the async service performs on the window.history api DO NOT work. If GTM and its relevant dependencies load quickly and before router has managed to be created the services work as expected.

This seems to be due to the way window.history methods are cloned in the createBrowser method of the history module.

Your Example Website or App

https://stackblitz.com/edit/vitejs-vite-jsq25s?file=src%2FApp.tsx

Steps to Reproduce the Bug or Issue

  1. Go to reproduction site
  2. Follow the chain of logs to see the ordering of cloning history methods.
  3. Click on the links and navigate around
  4. See that no console.logs are fired by my "naive" analytics implementation

Expected behavior

As a user When I implement an async services e.g. GTM that extends/overwrites the native browser apis e.g. history Then I expect them to work as described in their documentation

Currently they do not unless loaded before router/history is created (which can be detrimental to page loading).

Screenshots or Videos

No response

Platform

Additional context

Relevant code - early cloned history methods: https://github.com/TanStack/router/blob/main/packages/history/src/index.ts#L203 https://github.com/TanStack/router/blob/main/packages/history/src/index.ts#L204

X async service is able to overwrite and use the routers window.history changes as expected due to the following lines:

https://github.com/TanStack/router/blob/main/packages/history/src/index.ts#L317 https://github.com/TanStack/router/blob/main/packages/history/src/index.ts#L323

BUT by this point its to late as the router has already cloned the "native" implementation.

Follow the log lines in the reproduction.

Note: references to GTM are referring to Google Tag Manager.

schiller-manuel commented 3 weeks ago

I guess you just need to delay calling createRouter after Google Tag Manager is initialized. Apparently you can listen to the following event that tells you GTM is initialized:


window.addEventListener('gtm.js', function() {
   // createRouter ...
});
SeanCassiere commented 2 weeks ago

... Apparently you can listen to the following event that tells you GTM is initialized:

Building on this, since you need the GTM script to run first, you could just have a skeleton or something in your index.html inside the div#root. Once the GTM has loaded in and is initialized, you could call ReactDOM's createRoot and then initialise React on the client.

GregoryCollett commented 2 weeks ago

Thanks for your responses. I had done this but as mentioned, by "waiting" for gtm we are actively delaying the load of the application which in my opinion is an undesirable behaviour and does not seem like a valid solution. GTM could in theory take an infinite time to load. I would not want to delay loading of the application...

GregoryCollett commented 2 weeks ago

I guess you just need to delay calling createRouter after Google Tag Manager is initialized. Apparently you can listen to the following event that tells you GTM is initialized:

window.addEventListener('gtm.js', function() {
   // createRouter ...
});

I would disagree, as mentioned above this would mean delaying the loading of the application which is ultimately detrimental to the experience.

GregoryCollett commented 2 weeks ago

... Apparently you can listen to the following event that tells you GTM is initialized:

Building on this, since you need the GTM script to run first, you could just have a skeleton or something in your index.html inside the div#root. Once the GTM has loaded in and is initialized, you could call ReactDOM's createRoot and then initialise React on the client.

The thing is, I do not need the GTM script to load first. I personally think the way history is controlled by router is the problematic area here. Having GTM loaded async is standard practice. The cloning of window.history in the history module is the problem as it becomes out of sync with the "latest" implementation

tannerlinsley commented 2 weeks ago

I can see if this is addressable asap. The entire idea was that the history abstraction and the router were NOT to be the source of truth. I think that in most ways this is true aside from monkey patching the window history. I can probably work around this either by modifying the things to load later (though I’m not sure how much run we have) or by adding in a compat mode that doesn’t rely on the patch.

Either way, expect a fair delay in getting this fixed, but I’ll try to exceed expectation.

GregoryCollett commented 2 weeks ago

Hi @tannerlinsley thank you for your response. That makes sense.

I appreciate the discussion occurring in the discord also!

I had a few ideas on how to remedy this but I would need to formulate into a pr, happy to discuss ideas on it though!