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

Navigation storm in angular pilets #672

Open
3 tasks done
LBraeschke opened this issue Feb 8, 2024 · 1 comment
Open
3 tasks done

Navigation storm in angular pilets #672

LBraeschke opened this issue Feb 8, 2024 · 1 comment
Labels
angular Concerns the Angular integration piral-ng. bug Something isn't working help wanted Extra attention is needed

Comments

@LBraeschke
Copy link
Contributor

Bug Report

For more information, see the CONTRIBUTING guide.

Prerequisites

Environment Details and Version

Piral 1.4.3
Framework: Angular
OS: Windows 10
Browser Chrome

Description

[Description of the bug]

Steps to Reproduce

  1. checkout https://github.com/LBraeschke/piral-angular-nav-loop-bug/tree/master
  2. go to piral-debug-webpack5 and run npm install
  3. build emulator by running piral build --type emulator
  4. go to pilet-angular-webpack5 and run npm install
  5. run npm run start
  6. open browser at http://localhost:1234
  7. press the 'To Sharing' link

Expected behavior

Navigation is only executed 15 times

Actual behavior

Navigation is executed way more often. It seems it is realted to the computing power of the machine. On faster machine it is way more unlikely to occur and also it resolve faster, after fewer navigations.

navigation_storm

[Optionally, share your idea to fix the issue]
My guess is the Prial-Ng RoutingService.ts

 const queueNavigation = (url: string) => {
        // I guess this is the problem
        // the navigation frame is not yet available therefore the next router event is also queued and the setup of the navigation storm is completed
        window.requestAnimationFrame(() => nav.push(url));
      };

....
      
      this.subscription = this.router.events.subscribe((e: NavigationError | NavigationStart | Scroll) => {
       const locationUrl = nav.url;
....
       } else if (e.type === 15) {
          const index = skipIds.indexOf(e.routerEvent.id);

          if (index === -1) {
            // consistency check to avoid #535 and other Angular-specific issues
            const locationUrl = nav.url;
            const routerUrl = e.routerEvent.url;

            if (routerUrl !== locationUrl) {
              queueNavigation(routerUrl);
            }

My idea would be:

let targetUrl: string;
 const queueNavigation = () => {
        window.requestAnimationFrame(() => nav.push(targetUrl));
      };

....
      
      this.subscription = this.router.events.subscribe((e: NavigationError | NavigationStart | Scroll) => {
       const locationUrl = nav.url;
....
       } else if (e.type === 15) {
          const index = skipIds.indexOf(e.routerEvent.id);

          if (index === -1) {
            // consistency check to avoid #535 and other Angular-specific issues
            const locationUrl = nav.url;
            const routerUrl = e.routerEvent.url;

            if (routerUrl !== locationUrl) {
              targetUrl = routerUrl;
              queueNavigation();
            }
@LBraeschke LBraeschke added the bug Something isn't working label Feb 8, 2024
@FlorianRappl FlorianRappl added help wanted Extra attention is needed angular Concerns the Angular integration piral-ng. labels Feb 8, 2024
@FlorianRappl
Copy link
Contributor

PRs are on this one are welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular Concerns the Angular integration piral-ng. bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants