angular icon indicating copy to clipboard operation
angular copied to clipboard

feat(router): Create APIs for using Router without RouterModule

Open atscott opened this issue 3 years ago • 1 comments

This commit creates and exposes the APIs required to use the Angular Router without importing RouterModule.

By updating the golden test to use the new API, this change also demonstrates a powerful difference between the current RouterModule.forRoot(...) and the new provideRouter function: tree-shakability. The new opt-in API of the provideRouter function for features rather than using ExtraOptions to control the providers via a switch enables this. In truth, we could extend this ability to the RouterModule as well with something like forRootWithFeatures or by updating the existing forRoot to include the feature array.

TODOs:

  • [ ] Documentation
  • [x] RouterTestingModule equivalent (i.e. provideRouterForTesting)
  • [x] Determine how to expose configuration used by assignExtraOptionsToRouter. The new API only uses a subset of the ExtraOptions so the whole ExtraOptions config is not a good fit anymore, at the very least.
  • [x] Additional tests
  • [ ] Mark the new APIs as developer preview

atscott avatar Aug 01 '22 20:08 atscott

Open question for reviewers: When updating the tests in the router package to use the new provideRouterForTesting API, I kept using withXFeature without putting it in an array. Should we use a rest parameter for the features or should we keep it as an array in the second argument?

atscott avatar Aug 02 '22 23:08 atscott

You can preview 7b05799 at https://pr47010-7b05799.ngbuilds.io/.

mary-poppins avatar Aug 09 '22 00:08 mary-poppins

Really like that this makes the router features more tree-shakeable! Few questions:

  • Is it possible to have an object-based config to enable features and provide the same tree-shaking? This makes configuring the router much more verbose if you enable all the features.
  • What will the default be for a new application that uses this API? Are all the features included in the array, and you must delete them?
  • Will there be something to prevent using these provider features outside of the provideRouter function?
  • What about provideMockRouter or provideTestRouter instead of providerRouterForTesting

Thanks in advance!

brandonroberts avatar Aug 09 '22 13:08 brandonroberts

@brandonroberts Thanks for the feedback!

Is it possible to have an object-based config to enable features and provide the same tree-shaking? This makes configuring the router much more verbose if you enable all the features.

Yes, but the object-based configuration would actually be even more verbose. For example:

type RouterTracingFeature = Provider[];
function provideTracing(): RouterTracingFeature { … }

// Group all features in one interface:
type RouterFeatures = {
  tracing?: RouterTracingFeature,
  preloading?: RouterPreloadingFeature,
  …
};

// Usage
provideRouter([], {tracing: withTracing()});

The issue is that we can't have an if/else in the runtime code based on a runtime value. That makes both branches non-tree shakeable. The only way to do that in a way that would be tree shakeable would be to have compiler constants like ngDevMode.

Lastly, I think the verbosity actually isn't that much of an issue: enableTracing: true withDebugTracing() initialNavigation: 'disabled' withDisabledInitialNavigation()

Scrolling does require withInMemoryScrolling as a prefix for its options. But this change is worthwhile in my opinion because it namespaces the options to related the feature area rather than having a bunch of unrelated options live in one massive config object. The options in withRouterConfig aren't tree shakeable and don't really fall into a feature bucket. So those also require a slightly more verbose withRouterConfig as a prefix.

All that said, this config is only done once per application rather than being typed a bunch of times in various places. Additionally, I haven't found that it's super common to provide these options either, especially in simple apps.

What will the default be for a new application that uses this API? Are all the features included in the array, and you must delete them?

None of the features that have been split out are provided by default today in new applications. The only exception is the RouterScroller. But that only includes the event; you still have to opt-in to the scrolling behavior. So to answer the question directly, my feeling is that new applications would not have any of them. We could definitely consider adding some by default.

Will there be something to prevent using these provider features outside of the provideRouter function?

We certainly could consider this and do something like the trick for importProvidersFrom.

What about provideMockRouter or provideTestRouter instead of providerRouterForTesting

Happy to continue bikeshedding some options. You'll notice that the only distinguishing factor for the test router is the locations strategy used. I'll throw another option in there: provideRouter([], withTestingLocationStrategy()) (or another feature function with a similar name). This way, tests would look exactly the same as a regular application.

atscott avatar Aug 09 '22 15:08 atscott

@atscott

I was thinking something more like

import { provideRouter } from '@angular/router';

  provideRouter([], {
    enabletTracing: true,
    preloading: true
  })

vs

import { provideRouter, withDebugTracing, withPreloading } from '@angular/router';

  provideRouter([], [
    withDebugTracing(),
    withPreloading()
  ])

Additional imports are needed also, but if it can't be tree-shaken out by the compiler, I get it.

brandonroberts avatar Aug 09 '22 17:08 brandonroberts

You can preview 7db44bd at https://pr47010-7db44bd.ngbuilds.io/.

mary-poppins avatar Aug 09 '22 23:08 mary-poppins

You can preview f8ab94e at https://pr47010-f8ab94e.ngbuilds.io/.

mary-poppins avatar Aug 10 '22 17:08 mary-poppins

You can preview a0b4a6b at https://pr47010-a0b4a6b.ngbuilds.io/.

mary-poppins avatar Aug 12 '22 00:08 mary-poppins

You can preview df29701 at https://pr47010-df29701.ngbuilds.io/.

mary-poppins avatar Aug 12 '22 21:08 mary-poppins

You can preview 87b1b3e at https://pr47010-87b1b3e.ngbuilds.io/.

mary-poppins avatar Aug 12 '22 22:08 mary-poppins

Presubmit (TGP).

AndrewKushnir avatar Aug 13 '22 02:08 AndrewKushnir

FYI, TGP is "green", the PR is now ready for merge 🎉

AndrewKushnir avatar Aug 13 '22 05:08 AndrewKushnir

This PR was merged into the repository by commit 75df4044675c61d2b646437cfe64fe828a39b3a0.

alxhub avatar Aug 15 '22 22:08 alxhub

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.