thepassle / app-tools

129 stars 8 forks source link

Router: Open in new tab navigates to the correct url, then navigates to root #14

Closed JudahGabriel closed 1 year ago

JudahGabriel commented 1 year ago

I'm seeing some weirdness with "open in new tab" navigations.

Repro steps:

  1. Open your app at the root URL ("/")
  2. Middle-click (or CTRL+click) on a relative link on the page (/foo)
  3. In the new tab that opens, the new page opens and navigates to it (/foo). But after a second or so, it navigates again to the root (/).

This doesn't occur if I quickly switch to the new tab. It only occurs if I wait a few seconds before switching to the new tab.

Looking at where that 2nd phantom navigation comes from, it's from the router itself; nothing in my code spawns the 2nd nav.

I'll see if I can create a minimal repro shortly.

thepassle commented 1 year ago

I was unable to reproduce this locally in the index.html of the root of the project, but I have a feeling this might be because of the queueMicrotask https://github.com/thepassle/app-tools/blob/master/router/index.js#L53

I remember running into some timing issues with that, which is why I added it... I might have to add a install() method or something like that.

thepassle commented 1 year ago

Could you try to see if moving the following code in the Router class to a install method fixes the issue?

  constructor(config) {
    super();
    this.config = config;

    /** @type {Route[]} */
    this.routes = config.routes.map((route) => {
      const r = /** @type {unknown} */ ({
        ...route,
        // @ts-ignore
        urlPattern: new URLPattern({
          pathname: route.path,
          baseURL: window.location.href,
          search: '*',
          hash: '*',
        }),
      });
      return /** @type {Route} */ (r);
    });
    log('Initialized routes', this.routes);

  }

+  install() {
+    window.addEventListener('popstate', this._onPopState);
+    window.addEventListener('click', this._onAnchorClick);
+    this.navigate(new URL(window.location.href));
+  }

and then wherever you create the router:

const router = new Router(/* etc */);
+router.install();
JudahGabriel commented 1 year ago

Agreed it may be a timing thing.

Sure thing, I'll try this out.

JudahGabriel commented 1 year ago

Turns out it's a bug in my code. 😳

I had a nav bar that asynchronously loaded. When it was loaded in a background tab, there's a delay in when events fire...leading to some unexpected series of events that triggered home navigation.

Not a bug in your code! Closing.

thepassle commented 1 year ago

Thanks for confirming!

Would you mind trying out if the code I suggested wirks for your project either way? Id kind of like to not depend on the microtask now but make the installation of the router more explicit