fix(common): server-side rendering error when using in-memory scrolling

[crisbeto]

Fixes that the BrowserViewportScroller was throwing an error during server-side rendering because it was accessing window directly. Also removes some assertions that aren't necessary anymore.

Fixes #53682.

[alan-agius4]

Shouldn't actually withInMemoryScrolling be nooped instead when platform is server by returning null? I don't see why this feature should be even enabled for non browser platforms.

https://github.com/angular/angular/blob/d315e2c4fa178dfbd41bc25259605bb999fa302e/packages/router/src/provide_router.ts#L183:L188

[crisbeto]

In that case we would have to provide a noop implementation of the class. The current class already has all the necessary checks in place, the only missing one was in the factory.

[alan-agius4]

@crisbeto, I don't quite see the necessity for creating a noop implementation. Since ROUTER_SCROLLER is an optional token, returning null is acceptable. You can find the relevant code

angular/packages/router/src/provide_router.ts

Line 211 in d315e2c

injector.get(ROUTER_SCROLLER, null, InjectFlags.Optional)?.init();

Therefore, the following code should be adequate. The advantage here is that it avoids the creation and subscription to unused router events and can also contribute to better future-proofing, preventing potential regressions if a feature isn't utilized in SSR.

const providers = [{
  provide: ROUTER_SCROLLER,
  useFactory: () => {
+    const platformId = inject(PLATFORM_ID);
+    if (!isPlatformBrowser(platformId)) {
+       return null;
+    }
+
    const viewportScroller = inject(ViewportScroller);
    const zone = inject(NgZone);
    const transitions = inject(NavigationTransitions);
    const urlSerializer = inject(UrlSerializer);
    return new RouterScroller(urlSerializer, transitions, viewportScroller, zone, options);
  },
}];

I'm not strongly attached to this suggestion, but I lean towards not enabling unnecessary features.

[crisbeto]

My concern was that this is still going to error if somebody tries to inject ViewportScroller. Since it's providedIn: 'root', it doesn't necessarily have to be provided by the router in order to be injectable. The other problem is that the ROUTER_SCROLLER doesn't know how it's going to be injected. E.g. currently the only place we use it's optional, but if another place injects it later on, it'll be misleading to return null. Currently it's typed as InjectionToken<RouterScroller>, not InjectionToken<RouterScroller|null>.

[atscott]

This PR was merged into the repository by commit dd052dc.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}