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

Closed
3 tasks done
LBraeschke opened this issue Feb 8, 2024 · 3 comments
Closed
3 tasks done

Navigation storm in angular pilets #672

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

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 :)

@FlorianRappl FlorianRappl added the in-testing The item is already out in preview and can be tested. label Jun 25, 2024
@FlorianRappl
Copy link
Contributor

The preview 1.6.0 contains a suggestion to cancel the animation frame if another request pops in.

Let me know if this works for you @LBraeschke !

@FlorianRappl FlorianRappl added this to the 1.6.0 milestone Jun 25, 2024
@FlorianRappl
Copy link
Contributor

Any news here? Otherwise I'll close this one @LBraeschke .

@FlorianRappl FlorianRappl removed the in-testing The item is already out in preview and can be tested. label Jul 6, 2024
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