mgechev / ngx-quicklink

Quicklink prefetching strategy for the Angular router
MIT License
750 stars 43 forks source link

Idea: http data pre-fetching #30

Closed BioPhoton closed 4 years ago

BioPhoton commented 4 years ago

First THANKS A LOT for this great library.

I want to discuss an idea here.

When a certain part of the page gets prefetched it is most likely always associated with some data from the backend.

If we could provide a way where users can place a side effect for data prefetching the 2 things could happen in parallel. This would lead to a really nice performance boost for the time of meaningful interaction with the page.

I would not use router resolver or guards as it should really just be a side effect that can happen in parallel. The options are imho more for sequential actions.

Would this be an interesting thing to discuss?

Best, Michael

mgechev commented 4 years ago

I spent quite some time thinking about this feature. In general, I considered two separate solutions:

  1. When prefetching, go through the resolvers of the route and process them the same way the Angular router's logic does. This would allow downloading data during prefetch.
  2. Explose a service which via an observable notifies which routes are getting prefetched. This way the consumer of the library would be able to trigger these network requests.

The first option is very magical. It may not trigger the requests in the proper order.

The second option sounds fair, but at the same time the Angular's prefetching strategies are not designed with data prefetching in mind. I don't want to re-invent them with quicklink. Ideally, I'd also not want to couple the module with rxjs too much, nor extend its API surface.

Let's continue the discussion for a bit and see where we land since this feature request sounds reasonable.

BioPhoton commented 4 years ago

So focusing on 2. I have a first opinion: Notification about who gets loaded is definitely required. The problem though is I need the data in the lazy module.

Here the module has to expose the part where it holds or loads data as static in the main bundle...

Or i hold the data in some cache like stream and as soon as the module is loaded I forward the loaded data to the module.

Both feels clumsy.

mgechev commented 4 years ago

Or i hold the data in some cache like stream and as soon as the module is loaded I forward the loaded data to the module.

This sounds fine to me. If you fetch data and store it in a shared store this should be fine.

Flyrell commented 4 years ago

@mgechev I think the first option you proposed is also valid. It would be useful to hide it behind the flag in some property in route's data, like in the preload exclusion. It could be set to false by default.

Of course, you wouldn't set it when user actions change the data you rely on in the preloaded route.

const routes: Route[] = [
  {
    path: 'page',
    component: ComponentA,
    data: {
      preloadResolvers: true
    }
  }
];

The other solution I can think of is to export a simple directive which could be used with routerLink, so that we can set the preloading behaviour per page. Something like the following...

  <a [routerLink]="['', 'some', 'route']" preloadResolvers></a>

What do you think?

mgechev commented 4 years ago

I don't like flags much. Over time the API gets complicated, the flags are not orthogonal, and things start interfering with each other.

Flyrell commented 4 years ago

Thank you for your time. I see your point and totally agree. Unfortunately, I need functionality like this ASAP, so I think I'm still going to implement the flags in the fork for now when I have some time (and if I'll be able to, of course). If we figure out something better I'm gonna switch, I guess.

mgechev commented 4 years ago

@Flyrell what do you think about the other way to go - implement a Quicklink service which has an observable prefetch$ that emits when quicklink is about to prefetch a page?

Flyrell commented 4 years ago

You mean something like the following, right? What would I be able to do when the event occurs?

class ComponentA implements OnInit {

  constructor(private quickLinkService: QuickLinkService) {}

  onInit(): void {
    this.quickLinkService.onPrefetch().subscribe(preloadEvent => {
      // what do I do here?
      // something like `preloadEvent.cancel()` `preloadEvent.continue()` ?
    });
  }
}

Seems reasonable, but I'm not sure I understand the usage correctly.

mgechev commented 4 years ago

Yes, these are possible options. You can cancel the preload and also load data that the preloaded route needs.

Flyrell commented 4 years ago

Well, then I'm 100% in. Just tell me what you need to help with 😉

mgechev commented 4 years ago

Thanks! Our main goal is to not make everything too magical and keep the library's footprint small. I'd suggest defining a service which exposes an observable called prefetch$. We can export it as a provider in the QuicklinkModule and let folks to inject it in their apps.

They'd be able to consume it this way:

import { Quicklink } from 'ngx-quicklink';

@Component(...)
export class AppComponent {
  constructor(private _ql: Quicklink) { }

  ngOnInit() {
    this._ql.prefetch$.subscribe((r: Route) => {
      // perform an action
    });
  }
}

Would you want to open a PR with an implementation? Would love to review and provide feedback!

Flyrell commented 4 years ago

Yeah sure, the problem is I don't know when I'd have some spare time. Hope today/tomorrow, but I'm not promising anything.

Flyrell commented 4 years ago

Well, I had a few minutes to look at the source code and it looks like Angular has no API/option to do such a thing as resolver preloading. User would need to duplicate the logic or move away from the resolvers in order to do the prefetching based on the used strategy.

In my opinion this is not a valid solution and right now I don't see a way to implement it the way I'd want it to work. If I have some time tomorrow, I'll try to look at the code base once again and maybe I'll find the way to do this. Meanwhile, I opened the issue (feature request) on the @angular/router repository to add the option to preload the resolvers. Feel free to add your opinions.

mgechev commented 4 years ago

Yes, resolvers cannot be shared between quicklink and the router, but there could be a shared service between the resolvers and quicklink which can preload data.

mgechev commented 4 years ago

Closing for now. Let's keep the scope of the module small.