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

feature: support reusing shell but not reusing its children #431

Open
1 of 3 tasks
captaincaius opened this issue Jan 16, 2019 · 8 comments
Open
1 of 3 tasks

feature: support reusing shell but not reusing its children #431

captaincaius opened this issue Jan 16, 2019 · 8 comments
Labels

Comments

@captaincaius
Copy link
Contributor

captaincaius commented Jan 16, 2019

EDIT: I noticed that in my half-asleep state, my example wasn't clear, so I cleared it up a bit.

I'm submitting a...

  • Bug report
  • Feature request
  • Documentation issue or request

Current behavior

Our routeReuseStrategy out of the box is set up to reuse everything. But some people (like me) might want only the shell to be reused by default, and only be able to opt-in components underneath it to avoid the burden of having to subscribe to route parameters (or worse, route events if you need query params).

Expected behavior

Route from /search?something=this to /search?something=that and simply rely on the component to be destroyed and reinitialized. But the shell should be reused.

A simple working hack that keeps it optional is something like this:

route reuse strategy:

  public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
    return ((!curr.parent || !curr.parent.data || !curr.parent.data.destroyChildren) && future.routeConfig === curr.routeConfig) || future.data.reuse;
  }

shell service

  static childRoutes(routes: Routes): Route {
    return {
      path: '',
      component: ShellComponent,
      children: routes,
      runGuardsAndResolvers: 'always',
      canActivate: [AuthenticationGuard],
      // Reuse ShellComponent instance when navigating between child views
      data: { reuse: true, destroyChildren: true },
    };
  }

I personally prefer this because I don't like having some pages use the activated route snapshot and others subscribe to the observables. Subscribing to the route observables at the routed-component level feels a lot like a wrong separation of concerns to me. I'm sure this opinion is application-specific, though, and it's easy enough to support both while still always keeping the shell alive.

If you think it's worth having I'm sure there's cleaner ideas.

Minimal reproduction of the problem with instructions

Environment

- generator version: release/6.x
- node version: 10.14.2  <!-- run `node --version` -->
- npm version: 6.4.1  <!-- run `npm --version` -->
- OS: Linux <!-- Mac, Linux, Windows -->

Others:

@captaincaius
Copy link
Contributor Author

sigh i just looked at this and noticed my example didn't match my setup (forgot an inversion) ... Fixing again

@sinedied
Copy link
Member

If I understand it well, you are saying that for example if I navigate to from /shell/home to /shell/about, then back to /shell/home, home component is not destroyed/rebuilt?

If yes, that's definitely a bug I think 😕
The initial intent was to work around this bug (angular/angular#18374) to only reuse the common parent component(s) when navigating between children.

@captaincaius
Copy link
Contributor Author

captaincaius commented Jan 30, 2019

Ah. Sorry I wasn't clear. No, the scenario you listed currently does work right.

What I meant was that if you navigate
from /shell/about/234789 to /shell/about/298731
or from /shell/about/234789 to /shell/about/234789?filter=blah
...then the about component is reused.

One may or may not want this, but I prefer "shell" to be reused, but "about" to be destroyed in these two cases - then no components below the shell ever need to subscribe to router events just to know their params - to me it's worth the slight wastefulness ;).

I do understand that not everybody will share that opinion, so I figured it's easy enough to leave both options in the RouteReuseStrategy. And yes, this approach still works around the shell-gets-destroyed thing from the issue you posted ;) - it just slightly augments it.

@sinedied
Copy link
Member

sinedied commented Feb 1, 2019

I understand better now, thanks for the clarification!
Then I think the best solution would be to keep the reuse specific, ie only for the route level it's defined for, so we do not add a new option but make the current one work only for 1 level (the shell in this case) to it's both simpler and less error-prone.
And I had the issue with the route query params with ActivateRoute and was wondering why it did not get updated, now I know why 😄

@captaincaius
Copy link
Contributor Author

Ha. Okay that's how I've done it myself, but I was afraid I was being too opinionated.

I'll submit a PR - I'm pretty sure it won't conflict with your ionic update.

@captaincaius
Copy link
Contributor Author

Darn.

I don't want to make a hasty PR without doing some experimentation first.

After looking more closely I realized the approach I'm using to fix this might not work for ngx-rocket because I'm using the shell component for ALL routes - authenticated ones and anonymous ones - I do my auth-guarding stuff one level lower.

@niraj114
Copy link

niraj114 commented Apr 17, 2020

Hi,

I am facing below issues while implementing lazy loading in my applicaton. Please can you guide me. How to fix this issue.

Uncaught (in promise): Error: Component ShellComponent is not part of any NgModule or the module has not been imported into your module.

@sinedied
Copy link
Member

@niraj114 please open a separate issue for this following the template with your environment information so we can help you, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants