[router] plugin hook before rendering

[daKmoR]

I would like to fully load an api request before doing the actual rendering/navigation.

what should happen

1. showing a list of clients
2. click on one item in the list
3. fetching starts - a global loading indicator is show but no change to current page (e.g. no render)
4. fetching finished - render + navigate

what happens

3. fetching starts - page is updated with nothing or a generic loading message (e.g. whatever you decide in your render)
4. fetching finished - navigation / url change happens

ideas

I tried the beforeNavigation but that instantly renders and results in the what happens output

/**
 * @param {(context: import('@thepassle/app-tools/router.js').Context) => Promise<object>} promise
 * @returns {import('@thepassle/app-tools/router.js').Plugin}
 */
export function resolveData(promise) {
  return {
    name: 'data',
    beforeNavigation: async context => {
      const data = await promise(context);
      context.data = data;
      // + probably some error handling
    },
  };
}

how about adding a beforeRender hook? then the above code will work fine with one line change

-     beforeNavigation: async context => {
+     beforeRender: async context => {

code idea

this.route could be split into this.renderRoute and this.route and renderRoute would only be updated after all beforeRender hook have been executed.

usage example

    {
      path: '/client/:id',
      title: 'Client',
      plugins: [
        resolveData(context => api.get(`v1/client?client-id=${context.params.id}`)),
      ],
      render: context => {
        // context.data is always available - e.g. navigation would not happen if api call result in an error
        return html`<page-client .data=${context.data}></page-client>`;
      },
    },

what do you think?

[thepassle]

Rendering and navigations are decoupled; the navigation always happens (because thats how the web works, I'm not aware of any router that allows you to cancel the navigation entirely and just have nothing happen when a link is clicked, and I also don't think you should, it feels very un-web-like). But you can still show a global spinner, and only render the new page when the promise resolves by awaiting the promise in the route-changed event. The actual navigation would still happen, though, but I think it should. I think it's weird to click an anchor and then nothing happens.

Alternatively you can try using the shouldNavigate hook for this, pseudocode, but something like:

export function resolveData(promise) {
  return {
    name: 'resolveData',
    shouldNavigate: (context) => ({
      condition: async () => {
        let result = false;
        try {
          context.data = await promise();
          result = true;
        } catch {
          result = false;
        }
        return result;
      },
      redirect: '/client-list',
    })
  }
}

Here's a playground url: https://lit.dev/playground/#project=W3sibmFtZSI6InNpbXBsZS1ncmVldGluZy5qcyIsImNvbnRlbnQiOiJpbXBvcnQge2h0bWwsIGNzcywgTGl0RWxlbWVudH0gZnJvbSAnbGl0JztcbmltcG9ydCB7IFJvdXRlciB9IGZyb20gJ0B0aGVwYXNzbGUvYXBwLXRvb2xzL3JvdXRlci5qcyc7XG5cbmNvbnN0IHNsZWVwID0gKHN1Y2Nlc3MpID0-IG5ldyBQcm9taXNlKChyZXNvbHZlLCByZWplY3QpID0-IHNldFRpbWVvdXQoc3VjY2VzcyA_IHJlc29sdmUgOiByZWplY3QsIDEwMDApKTtcblxuZnVuY3Rpb24gZGF0YShwcm9taXNlKSB7XG4gIHJldHVybiB7XG4gICAgc2hvdWxkTmF2aWdhdGU6IChjb250ZXh0KSA9PiAoe1xuICAgICAgY29uZGl0aW9uOiBhc3luYyAoKSA9PiB7XG4gICAgICAgIGxldCByZXN1bHQgPSBmYWxzZTtcbiAgICAgICAgdHJ5IHtcbiAgICAgICAgICBjb250ZXh0LmRhdGEgPSBhd2FpdCBwcm9taXNlKCk7XG4gICAgICAgICAgcmVzdWx0ID0gdHJ1ZTtcbiAgICAgICAgfSBjYXRjaCB7XG4gICAgICAgICAgY29uc29sZS5sb2coJ2ZhaWxlZCcpO1xuICAgICAgICB9XG4gICAgICAgIHJldHVybiByZXN1bHQ7XG4gICAgICB9LFxuICAgICAgcmVkaXJlY3Q6ICcnXG4gICAgfSksXG4gIH1cbn1cblxuY29uc3Qgcm91dGVyID0gbmV3IFJvdXRlcih7XG4gIHBsdWdpbnM6IFtcbiAgICBcbiAgXSxcbiAgcm91dGVzOiBbXG4gICAge1xuICAgICAgcGF0aDogJycsXG4gICAgICB0aXRsZTogJ0hlbGxvJyxcbiAgICAgIHJlbmRlcjogKCkgPT4gaHRtbGBcbiAgICAgICAgPHVsPlxuICAgICAgICAgIDxsaT48YSBocmVmPVwiaXRlbS9mb29cIj5mb288L2E-PC9saT5cbiAgICAgICAgICA8bGk-PGEgaHJlZj1cIml0ZW0vYmFyXCI-YmFyPC9hPjwvbGk-XG4gICAgICAgIDwvdWw-XG4gICAgICBgXG4gICAgfSxcbiAgICB7XG4gICAgICBwYXRoOiAnaXRlbS86aXRlbScsXG4gICAgICBwbHVnaW5zOiBbXG4gICAgICAgIGRhdGEoKCkgPT4gc2xlZXAoTWF0aC5yYW5kb20oKSA8IDAuNSkpXG4gICAgICBdLFxuICAgICAgdGl0bGU6ICdpdGVtJyxcbiAgICAgIHJlbmRlcjogKGNvbnRleHQpID0-IGNvbnRleHQucGFyYW1zLml0ZW1cbiAgICB9XG4gIF1cbn0pO1xuXG5leHBvcnQgY2xhc3MgU2ltcGxlR3JlZXRpbmcgZXh0ZW5kcyBMaXRFbGVtZW50IHtcbiAgZmlyc3RVcGRhdGVkKCkge1xuICAgIHJvdXRlci5hZGRFdmVudExpc3RlbmVyKCdyb3V0ZS1jaGFuZ2VkJywgKCkgPT4ge1xuICAgICAgdGhpcy5yZXF1ZXN0VXBkYXRlKCk7XG4gICAgfSk7XG4gIH1cblxuICByZW5kZXIoKSB7XG4gICAgcmV0dXJuIHJvdXRlci5yZW5kZXIoKTtcbiAgfVxufVxuY3VzdG9tRWxlbWVudHMuZGVmaW5lKCdzaW1wbGUtZ3JlZXRpbmcnLCBTaW1wbGVHcmVldGluZyk7XG4ifSx7Im5hbWUiOiJpbmRleC5odG1sIiwiY29udGVudCI6IjwhRE9DVFlQRSBodG1sPlxuPGhlYWQ-XG4gIDxzY3JpcHQgdHlwZT1cIm1vZHVsZVwiIHNyYz1cIi4vc2ltcGxlLWdyZWV0aW5nLmpzXCI-PC9zY3JpcHQ-XG48L2hlYWQ-XG48Ym9keT5cbiAgPHNpbXBsZS1ncmVldGluZyBuYW1lPVwiV29ybGRcIj48L3NpbXBsZS1ncmVldGluZz5cbjwvYm9keT5cbiJ9LHsibmFtZSI6InBhY2thZ2UuanNvbiIsImNvbnRlbnQiOiJ7XG4gIFwiZGVwZW5kZW5jaWVzXCI6IHtcbiAgICBcImxpdFwiOiBcIl4yLjAuMFwiLFxuICAgIFwiQGxpdC9yZWFjdGl2ZS1lbGVtZW50XCI6IFwiXjEuMC4wXCIsXG4gICAgXCJsaXQtZWxlbWVudFwiOiBcIl4zLjAuMFwiLFxuICAgIFwibGl0LWh0bWxcIjogXCJeMi4wLjBcIlxuICB9XG59IiwiaGlkZGVuIjp0cnVlfV0

[daKmoR]

I was totally confused as I saw it working in the playground but I could not get it to work in my codebase...

but I think I have narrowed down the issue... even with shouldNavigate it still changes the this.route which makes sense... but if then any other event triggers a rerender of the main app... (like a loading indicator)

then it will render the new route even though shouldNavigate is not yet done and then depending on your template it may error or will need to show some loading state (as data is not yet available)

you can see it in action here

https://lit.dev/playground/#project=W3sibmFtZSI6InNpbXBsZS1ncmVldGluZy5qcyIsImNvbnRlbnQiOiJpbXBvcnQge2h0bWwsIGNzcywgTGl0RWxlbWVudH0gZnJvbSAnbGl0JztcbmltcG9ydCB7IFJvdXRlciB9IGZyb20gJ0B0aGVwYXNzbGUvYXBwLXRvb2xzL3JvdXRlci5qcyc7XG5cbmNvbnN0IHNsZWVwID0gKHN1Y2Nlc3MpID0-IG5ldyBQcm9taXNlKChyZXNvbHZlLCByZWplY3QpID0-IHNldFRpbWVvdXQoc3VjY2VzcyA_IHJlc29sdmUgOiByZWplY3QsIDEwMDApKTtcblxuZnVuY3Rpb24gZGF0YShwcm9taXNlKSB7XG4gIHJldHVybiB7XG4gICAgc2hvdWxkTmF2aWdhdGU6IChjb250ZXh0KSA9PiAoe1xuICAgICAgY29uZGl0aW9uOiBhc3luYyAoKSA9PiB7XG4gICAgICAgIGxldCByZXN1bHQgPSBmYWxzZTtcbiAgICAgICAgdHJ5IHtcbiAgICAgICAgICBkb2N1bWVudC5xdWVyeVNlbGVjdG9yKCdzaW1wbGUtZ3JlZXRpbmcnKS5sb2FkaW5nID0gdHJ1ZTtcbiAgICAgICAgICBjb250ZXh0LmRhdGEgPSBhd2FpdCBwcm9taXNlKCk7XG4gICAgICAgICAgcmVzdWx0ID0gdHJ1ZTtcbiAgICAgICAgfSBjYXRjaCB7XG4gICAgICAgICAgY29uc29sZS5sb2coJ2ZhaWxlZCcpO1xuICAgICAgICB9XG4gICAgICAgIGRvY3VtZW50LnF1ZXJ5U2VsZWN0b3IoJ3NpbXBsZS1ncmVldGluZycpLmxvYWRpbmcgPSBmYWxzZTtcbiAgICAgICAgcmV0dXJuIHJlc3VsdDtcbiAgICAgIH0sXG4gICAgICByZWRpcmVjdDogJydcbiAgICB9KSxcbiAgfVxufVxuXG5jb25zdCByb3V0ZXIgPSBuZXcgUm91dGVyKHtcbiAgcGx1Z2luczogW1xuICAgIFxuICBdLFxuICByb3V0ZXM6IFtcbiAgICB7XG4gICAgICBwYXRoOiAnJyxcbiAgICAgIHRpdGxlOiAnSGVsbG8nLFxuICAgICAgcmVuZGVyOiAoKSA9PiBodG1sYFxuICAgICAgICA8dWw-XG4gICAgICAgICAgPGxpPjxhIGhyZWY9XCJpdGVtL2Zvb1wiPmZvbzwvYT48L2xpPlxuICAgICAgICAgIDxsaT48YSBocmVmPVwiaXRlbS9iYXJcIj5iYXI8L2E-PC9saT5cbiAgICAgICAgPC91bD5cbiAgICAgIGBcbiAgICB9LFxuICAgIHtcbiAgICAgIHBhdGg6ICdpdGVtLzppdGVtJyxcbiAgICAgIHBsdWdpbnM6IFtcbiAgICAgICAgZGF0YSgoKSA9PiBuZXcgUHJvbWlzZSgocmVzKSA9PiBzZXRUaW1lb3V0KCgpID0-IHJlcyh7IHNvbWU6ICdkYXRhJyB9KSwgMTAwMCkpKVxuICAgICAgXSxcbiAgICAgIHRpdGxlOiAnaXRlbScsXG4gICAgICByZW5kZXI6IChjb250ZXh0KSA9PiB7XG4gICAgICAgIGlmICghY29udGV4dC5kYXRhKSByZXR1cm4gaHRtbGBTdGlsbCBGZXRjaGluZz9gO1xuICAgICAgICByZXR1cm4gaHRtbGBcbiAgICAgICAgICAgIDxoMT5JdGVtPC9oMT5cbiAgICAgICAgICAgICR7Y29udGV4dC5kYXRhLnNvbWV9XG4gICAgICAgICAgICAke2NvbnRleHQucGFyYW1zLml0ZW19XG4gICAgICAgIGA7XG4gICAgICB9XG4gICAgfVxuICBdXG59KTtcblxuZXhwb3J0IGNsYXNzIFNpbXBsZUdyZWV0aW5nIGV4dGVuZHMgTGl0RWxlbWVudCB7XG4gIHN0YXRpYyBwcm9wZXJ0aWVzID0ge1xuICAgIGxvYWRpbmc6IHsgdHlwZTogQm9vbGVhbiB9LFxuICB9O1xuXG4gIGNvbnN0cnVjdG9yKCkge1xuICAgIHN1cGVyKCk7XG4gICAgdGhpcy5sb2FkaW5nID0gZmFsc2U7XG4gIH1cbiAgXG4gIGZpcnN0VXBkYXRlZCgpIHtcbiAgICByb3V0ZXIuYWRkRXZlbnRMaXN0ZW5lcigncm91dGUtY2hhbmdlZCcsICgpID0-IHtcbiAgICAgIHRoaXMucmVxdWVzdFVwZGF0ZSgpO1xuICAgIH0pO1xuICB9XG5cbiAgcmVuZGVyKCkge1xuICAgIFxuICAgIHJldHVybiBodG1sYFxuICAgICAgICAke3RoaXMubG9hZGluZyA_IGh0bWxgR2xvYmFsIExvYWRpbmcuLi5gIDogJyd9XG4gICAgICAgICR7cm91dGVyLnJlbmRlcigpfVxuICAgIGA7XG4gIH1cbn1cbmN1c3RvbUVsZW1lbnRzLmRlZmluZSgnc2ltcGxlLWdyZWV0aW5nJywgU2ltcGxlR3JlZXRpbmcpO1xuIn0seyJuYW1lIjoiaW5kZXguaHRtbCIsImNvbnRlbnQiOiI8IURPQ1RZUEUgaHRtbD5cbjxoZWFkPlxuICA8c2NyaXB0IHR5cGU9XCJtb2R1bGVcIiBzcmM9XCIuL3NpbXBsZS1ncmVldGluZy5qc1wiPjwvc2NyaXB0PlxuPC9oZWFkPlxuPGJvZHk-XG4gIDxzaW1wbGUtZ3JlZXRpbmcgbmFtZT1cIldvcmxkXCI-PC9zaW1wbGUtZ3JlZXRpbmc-XG48L2JvZHk-XG4ifSx7Im5hbWUiOiJwYWNrYWdlLmpzb24iLCJjb250ZW50Ijoie1xuICBcImRlcGVuZGVuY2llc1wiOiB7XG4gICAgXCJsaXRcIjogXCJeMi4wLjBcIixcbiAgICBcIkBsaXQvcmVhY3RpdmUtZWxlbWVudFwiOiBcIl4xLjAuMFwiLFxuICAgIFwibGl0LWVsZW1lbnRcIjogXCJeMy4wLjBcIixcbiAgICBcImxpdC1odG1sXCI6IFwiXjIuMC4wXCJcbiAgfVxufSIsImhpZGRlbiI6dHJ1ZX1d

so if there is an external update then rendering and navigation seems to not be decoupled 😅 so maybe my original code idea is still the best way forward? 🤔

what do you think?

[thepassle]

so if there is an external update then rendering and navigation seems to not be decoupled 😅

Well it is decoupled, it just depends on when and where you're calling the render function. (And in this case how Lit works as well, because this.route is updated sync before shouldNavigate updates this.route)

even with shouldNavigate it still changes the this.route

This is a fair point though, but this could be remedied by moving execution of the shouldNavigate hook to before assigning the new route, which I think makes more sense actually. Currently, we assign the new route (this.route) synchronously, then execute shouldNavigate which is async and gives lit time to render (and this.route is now already the new route, so it'll be re-rendered by lit), and may also re-assign the route, which is why I think you're seeing the route rendered. It's kind of weird to already assign the route if we dont even know if we shouldNavigate there or not.

If I understand correctly, this is the expected behavior you want to see, correct?

https://github.com/thepassle/app-tools/assets/17054057/3578fe93-6a69-4dff-893b-716043a0bd1d

If so, we can change the assigning of this.route to move after shouldNavigate has happened:

--- a/router/index.js
+++ b/router/index.js
@@ -165,13 +165,13 @@ export class Router extends EventTarget {
       url = new URL(url, this.baseUrl);
     }

-    this.route = this._matchRoute(url) || this._matchRoute(this.fallback);
+    let route = this._matchRoute(url) || this._matchRoute(this.fallback);
     log(`Navigating to ${url.pathname}${url.search}`, { context: this.context, route: this.route });

     /** @type {Plugin[]} */
     const plugins = [
       ...(this.config?.plugins ?? []), 
-      ...(this.route?.plugins ?? []), 
+      ...(route?.plugins ?? []), 
     ];

     for (const plugin of plugins) {
@@ -181,7 +181,7 @@ export class Router extends EventTarget {
           const condition = await result.condition();
           if (!condition) {
             url = new URL(result.redirect, this.baseUrl);
-            this.route = this._matchRoute(url) || this._matchRoute(this.fallback);
+            route = this._matchRoute(url) || this._matchRoute(this.fallback);
             log('Redirecting', { context: this.context, route: this.route });
           }
         }
@@ -191,6 +191,8 @@ export class Router extends EventTarget {
       }
     }

+    this.route = route;

^ I think something like this should work

[daKmoR]

yes that is the behaviour I would like to get 👍

tried the change but it seems to bring another "problem" e.g. if lit rerenders while the route changes then it will rerender the "old" route - but somehow the old context.data is no longer available 🤷

so now the "new" route works as expected (e.g. wait until data is there and then render)... but the "old" route has missing data.

e.g. that only applies if old and new route both depend on data (which is probably often the case)

See example here with the changes to the Router.js (new file) from above

Playground Link

PS: this also highlight that for the initial rendering (from the app side) the contex.data will always be empty 😅 e.g. you need a condition in the render to account for that... would be nice if that won't be needed

[thepassle]

tried the change but it seems to bring another "problem" e.g. if lit rerenders while the route changes then it will rerender the "old" route - but somehow the old context.data is no longer available 🤷

Right, because if a redirect (a new assignment of the route) happens in shouldNavigate it wont execute the shouldNavigate hook for the route thats being redirected to.

So then if a redirect has happened, we may have to run the shouldNavigate hook for that route, too

[daKmoR]

huh - there is no redirect involved? at least not intentional 🤔 if I remove all redirects inside shouldNavigate it still behaves the same...

or do you mean any route change is a "redirect"?

[thepassle]

Maybe I'm misunderstanding then, can you clarify? Why is context.data empty?

[daKmoR]

I don't know why it's empty... if it would be there then it would work as expected 😅

in the Playground Link

if you click on John then it renders the "loading message" of the hello page... which is strange as that page is currently displayed and already has it's data loaded.... so somehow on the navigation it resets the context maybe? I can maybe do some debugging this evening but right now I'm confused 😅

[thepassle]

Right, so:

• When you click "John", this.navigate happens
• in this.navigate we do a new this._matchRoute, which creates a new this.context for the route we're navigating to (item/:item)
• Because you're setting document.querySelector('simple-greeting').loading, that triggers a new render, so the route.render is called for the Hello route, but the context has already changed

You can avoid the rerendering that happens when the loading state is set by assigning the result of route.render() to a property on the class:

  static properties = {
    loading: { type: Boolean },
+    route: {}
  };

  firstUpdated() {
    router.addEventListener('route-changed', () => {
-       this.requestUpdate();
+      this.route = router.render();
    });
  }

  render() {
    return html`
        ${this.loading ? html`Global Loading...` : ''}
-        ${router.render()}
+        ${this.route}
    `;
  }

Playground

Then it should only call the render callback for a route whenever it has actually changed:

https://github.com/thepassle/app-tools/assets/17054057/50827df9-7981-480a-8a3e-e9a1585353db

[daKmoR]

sweet - that works 💪

also less rerendering of the route 👍

[thepassle]

Fixed in 0.9.7, and published :)

[daKmoR]

updated and works 🎉

always a pleasure doing business with you 🤗