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

Deprecate class and InjectionToken guards and resolvers #47924

Closed

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Oct 31, 2022

Class and InjectionToken-based guards and resolvers are not as configurable,
are less re-usable, require more boilerplate, cannot be defined inline with the route,
and require more in-depth knowledge of Angular features (Injectable/providers).
In short, they're less powerful and more cumbersome.

In addition, continued support increases the API surface which in turn increases
bundle size, code complexity, the learning curve and API surface to teach,
maintenance cost, and cognitive load (needing to grok several different types
of information in a single place).

Lastly, supporting only the CanXFn types for guards and ResolveFn type
for resolvers in the Route interface will enable better code
completion and integration with TypeScript. For example, when writing an
inline functional resolver today, the function is typed as any and
does not provide completions for the ResolveFn parameters. By
restricting the type to only ResolveFn, in the example below
TypeScript would be able to correctly identify the route parameter as
ActivatedRouteSnapshot and when authoring the inline route, the
language service would be able to autocomplete the function parameters.

const userRoute: Route = {
  path: 'user/:id',
  resolve: {
    "user": (route) => inject(UserService).getUser(route.params['id']);
  }
};

Importantly, this deprecation only affects the support for class and
InjectionToken guards at the Route definition. Injectable classes
and InjectionToken providers are not being deprecated in the general
sense. Functional guards are robust enough to even support the existing
class-based guards through a transform:

function mapToCanMatch(providers: Array<Type<{canMatch: CanMatchFn}>>): CanMatchFn[] {
  return providers.map(provider => (...params) => inject(provider).canMatch(...params));
}
const route = {
  path: 'admin',
  canMatch: mapToCanMatch([AdminGuard]),
};

With regards to tests, because of the ability to map Injectable
classes to guard functions as outlined above, nothing needs to change
if projects prefer testing guards the way they do today. Functional
guards can also be written in a way that's either testable with
runInContext or by passing mock implementations of dependencies.
For example:

export function myGuardWithMockableDeps(
  dep1 = inject(MyService),
  dep2 = inject(MyService2),
  dep3 = inject(MyService3),
) { }

const route = {
  path: 'admin',
  canActivate: [() => myGuardWithMockableDeps()]
}

// test file
const guardResultWithMockDeps = myGuardWithMockableDeps(mockService1, mockService2, mockService3);
const guardResultWithRealDeps = TestBed.inject(EnvironmentInjector).runInContext(myGuardWithMockableDeps);

@atscott atscott added aio: preview target: rc This PR is targeted for the next release-candidate labels Oct 31, 2022
@ngbot ngbot bot added this to the Backlog milestone Oct 31, 2022
@atscott atscott force-pushed the guardsAsFunctions_deprecateOldStuff branch 4 times, most recently from 95d34ad to c2301ae Compare October 31, 2022 18:25
@mary-poppins
Copy link

You can preview c2301ae at https://pr47924-c2301ae.ngbuilds.io/.

@atscott atscott force-pushed the guardsAsFunctions_deprecateOldStuff branch 2 times, most recently from 01aa776 to 2a3e13d Compare October 31, 2022 19:19
@mary-poppins
Copy link

You can preview 2a3e13d at https://pr47924-2a3e13d.ngbuilds.io/.

@atscott atscott force-pushed the guardsAsFunctions_deprecateOldStuff branch 2 times, most recently from 02d2792 to 8972dd5 Compare October 31, 2022 19:32
@mary-poppins
Copy link

You can preview 8972dd5 at https://pr47924-8972dd5.ngbuilds.io/.

@atscott atscott force-pushed the guardsAsFunctions_deprecateOldStuff branch from 8972dd5 to e804ae2 Compare October 31, 2022 19:45
@mary-poppins
Copy link

You can preview e804ae2 at https://pr47924-e804ae2.ngbuilds.io/.

@atscott atscott force-pushed the guardsAsFunctions_deprecateOldStuff branch from e804ae2 to 1e30eda Compare October 31, 2022 20:07
@mary-poppins
Copy link

You can preview 1e30eda at https://pr47924-1e30eda.ngbuilds.io/.

@fmalcher
Copy link
Contributor

Will the schematics be updated as well so that "ng generate guard" creates a function? Or will they be removed?

@atscott
Copy link
Contributor Author

atscott commented Oct 31, 2022

@fmalcher The schematics will be updated, but as a separate PR.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

reviewed-for: fw-core, public-api

@pullapprove pullapprove bot requested a review from dylhunn October 31, 2022 20:35
@DaSchTour
Copy link

DaSchTour commented Oct 31, 2022

What is wrong with class based guard and resolvers? Why are they deprecated?

I have the impression some new architecture ideas are rushed and forced onto the community with no reason.

@atscott
Copy link
Contributor Author

atscott commented Oct 31, 2022

What is wrong with class based guard and resolvers? Why are they deprecated?

I have the impression some new architecture ideas are rushed and forced onto the community with no reason.

Class and InjectionToken-based guards and resolvers are not as configurable, are less re-usable, require more boilerplate, cannot be defined inline with the route, and require more in-depth knowledge of Angular features (Injectable/providers). In short, they're less powerful and more cumbersome.

In addition, continued support increases the API surface which in turn increases bundle size, code complexity, the learning curve and API surface to teach, maintenance cost, and cognitive load (needing to grok several different types of information in a single place).

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

nit: this line looks a bit complex:

const url = router.getCurrentNavigation()!.extractedUrl.toString();

I was thinking if we can replace it with something like this:

route = inject(ActivatedRoute).snapshot;
// ...later in the code...
this.route.url;

(not a blocker for this PR, feel free to ignore)

@atscott
Copy link
Contributor Author

atscott commented Oct 31, 2022

nit: this line looks a bit complex:

const url = router.getCurrentNavigation()!.extractedUrl.toString();

I was thinking if we can replace it with something like this:

route = inject(ActivatedRoute).snapshot;
// ...later in the code...
this.route.url;

Copying from chat: That doesn't quite work. ActivatedRoute represents the current activated route. This is a guard for an in-flight navigation, so that doesn't work.
For canActivate, you can write canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => state.url.
Comparatively, canMatch does not have access to RouterStateSnapshot so I can't write one function and share it between canMatch and canActivate unless we find a common way to get the URL. The previous version had

canMatch(route: Route) => `/${route.path}`

but that's a bit of a hack and also not available in canActivate.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 23, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.5/15.2.0) |
| [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.5/15.2.0) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.5/15.2.0) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.5/15.2.0) |
| [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.5/15.2.0) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.5/15.2.0) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.5/15.2.0) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.5/15.2.0) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v15.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1520-2023-02-22)

[Compare Source](angular/angular@15.1.5...15.2.0)

#### Deprecations

#####

-   Class and `InjectionToken` guards and resolvers are
    deprecated. Instead, write guards as plain JavaScript functions and
    inject dependencies with `inject` from `@angular/core`.

#####

| Commit | Type | Description |
| -- | -- | -- |
| [926c35f4ac](angular/angular@926c35f) | docs | Deprecate class and InjectionToken and resolvers ([#&#8203;47924](angular/angular#47924)) |

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [54b24eb40f](angular/angular@54b24eb) | feat | Add loaderParams attribute to NgOptimizedImage ([#&#8203;48907](angular/angular#48907)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [0cf11167f1](angular/angular@0cf1116) | fix | incorrectly detecting forward refs when symbol already exists in file ([#&#8203;48988](angular/angular#48988)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [a154db8a81](angular/angular@a154db8) | feat | add ng generate schematic to convert declarations to standalone ([#&#8203;48790](angular/angular#48790)) |
| [345e737daa](angular/angular@345e737) | feat | add ng generate schematic to convert to standalone bootstrapping APIs ([#&#8203;48848](angular/angular#48848)) |
| [e7318fc758](angular/angular@e7318fc) | feat | add ng generate schematic to remove unnecessary modules ([#&#8203;48832](angular/angular#48832)) |

##### language-service

| Commit | Type | Description |
| -- | -- | -- |
| [4ae384fd61](angular/angular@4ae384f) | feat | Allow auto-imports of a pipe via quick fix when its selector is used, both directly and via reexports. ([#&#8203;48354](angular/angular#48354)) |
| [141333411e](angular/angular@1413334) | feat | Introduce a new NgModuleIndex, and use it to suggest re-exports. ([#&#8203;48354](angular/angular#48354)) |
| [d0145033bd](angular/angular@d014503) | fix | generate forwardRef for same file imports ([#&#8203;48898](angular/angular#48898)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [2796230e95](angular/angular@2796230) | fix | add `enum` in `mode` option in `standalone` schema ([#&#8203;48851](angular/angular#48851)) |
| [816e76a578](angular/angular@816e76a) | fix | automatically prune root module after bootstrap step ([#&#8203;49030](angular/angular#49030)) |
| [bdbf21d04b](angular/angular@bdbf21d) | fix | avoid generating imports with forward slashes ([#&#8203;48993](angular/angular#48993)) |
| [32cf4e5cb9](angular/angular@32cf4e5) | fix | avoid internal modules when generating imports ([#&#8203;48958](angular/angular#48958)) |
| [521ccfbe6c](angular/angular@521ccfb) | fix | avoid interrupting the migration if language service lookup fails ([#&#8203;49010](angular/angular#49010)) |
| [a40cd47aa7](angular/angular@a40cd47) | fix | avoid modifying testing modules without declarations ([#&#8203;48921](angular/angular#48921)) |
| [1afa6ed322](angular/angular@1afa6ed) | fix | don't add ModuleWithProviders to standalone test components ([#&#8203;48987](angular/angular#48987)) |
| [c98c6a8452](angular/angular@c98c6a8) | fix | don't copy animations modules into the imports of test components ([#&#8203;49147](angular/angular#49147)) |
| [8389557848](angular/angular@8389557) | fix | don't copy unmigrated declarations into imports array ([#&#8203;48882](angular/angular#48882)) |
| [f82bdc4b01](angular/angular@f82bdc4) | fix | don't delete classes that may provide dependencies transitively ([#&#8203;48866](angular/angular#48866)) |
| [759db12e0b](angular/angular@759db12) | fix | duplicated comments on migrated classes ([#&#8203;48966](angular/angular#48966)) |
| [ba38178d19](angular/angular@ba38178) | fix | generate forwardRef for same file imports ([#&#8203;48898](angular/angular#48898)) |
| [03fcb36cfd](angular/angular@03fcb36) | fix | migrate HttpClientModule to provideHttpClient() ([#&#8203;48949](angular/angular#48949)) |
| [2de6dae16d](angular/angular@2de6dae) | fix | migrate RouterModule.forRoot with a config object to use features ([#&#8203;48935](angular/angular#48935)) |
| [770191cf1f](angular/angular@770191c) | fix | migrate tests when switching to standalone bootstrap API ([#&#8203;48987](angular/angular#48987)) |
| [c7926b5773](angular/angular@c7926b5) | fix | move standalone migrations into imports ([#&#8203;48987](angular/angular#48987)) |
| [65c74ed93e](angular/angular@65c74ed) | fix | normalize paths to posix ([#&#8203;48850](angular/angular#48850)) |
| [6377487b1a](angular/angular@6377487) | fix | only exclude bootstrapped declarations from initial standalone migration ([#&#8203;48987](angular/angular#48987)) |
| [e9e4449a43](angular/angular@e9e4449) | fix | preserve tsconfig in standalone migration ([#&#8203;48987](angular/angular#48987)) |
| [ffad1b49d9](angular/angular@ffad1b4) | fix | reduce number of files that need to be checked ([#&#8203;48987](angular/angular#48987)) |
| [ba7a757cc5](angular/angular@ba7a757) | fix | return correct alias when conflicting import exists ([#&#8203;49139](angular/angular#49139)) |
| [49a7c9f94a](angular/angular@49a7c9f) | fix | standalone migration incorrectly throwing path error for multi app projects ([#&#8203;48958](angular/angular#48958)) |
| [584976e6c8](angular/angular@584976e) | fix | support --defaults in standalone migration ([#&#8203;48921](angular/angular#48921)) |
| [03f47ac901](angular/angular@03f47ac) | fix | use consistent quotes in generated imports ([#&#8203;48876](angular/angular#48876)) |
| [ebae506d89](angular/angular@ebae506) | fix | use import remapper in root component ([#&#8203;49046](angular/angular#49046)) |
| [40c976c909](angular/angular@40c976c) | fix | use NgForOf instead of NgFor ([#&#8203;49022](angular/angular#49022)) |
| [4ac25b2aff](angular/angular@4ac25b2) | perf | avoid re-traversing nodes when resolving bootstrap call dependencies ([#&#8203;49010](angular/angular#49010)) |
| [26cb7ab2e6](angular/angular@26cb7ab) | perf | speed up language service lookups ([#&#8203;49010](angular/angular#49010)) |

##### platform-browser

| Commit | Type | Description |
| -- | -- | -- |
| [bf4ad38117](angular/angular@bf4ad38) | fix | remove styles from DOM of destroyed components ([#&#8203;48298](angular/angular#48298)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [25e220a23a](angular/angular@25e220a) | fix | avoid duplicate TransferState info after renderApplication call ([#&#8203;49094](angular/angular#49094)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [31b94c762f](angular/angular@31b94c7) | feat | Add a withNavigationErrorHandler feature to provideRouter ([#&#8203;48551](angular/angular#48551)) |
| [dedac8d3f7](angular/angular@dedac8d) | feat | Add test helper for trigger navigations in tests ([#&#8203;48552](angular/angular#48552)) |

#### Special Thanks

Alan Agius, Alex Castle, Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dylan Hunn, Ikko Eltociear Ashimine, Ilyass, Jessica Janiuk, Joey Perrott, John Manners, Kalbarczyk, Kristiyan Kostadinov, Matthieu Riegler, Paul Gschwendtner, Pawel Kozlowski, Virginia Dooley, Walid Bouguima, cexbrayat and mgechev

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDkuMCIsInVwZGF0ZWRJblZlciI6IjM0LjE1Mi4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1793
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@vugar005
Copy link
Contributor

vugar005 commented Feb 23, 2023

What is wrong with class based guard and resolvers? Why are they deprecated?

I have the impression some new architecture ideas are rushed and forced onto the community with no reason.

First I thought same, but I think we can still outsource guards by export function from a different file or by method from service.

@AGiorgetti
Copy link

isn't the explicit use of inject() considered an anti-pattern? This way yout're hiding a lot of dependencies. It sounds very strange to me encourage such an approach.

@thomas-w-grant
Copy link

thomas-w-grant commented Feb 27, 2023

I am not usually one to join these sort of conversations, but I feel very motivated to do so now. I have gravitated toward Angular from the beginning because of its consistent preference to class constructor injection and class approach over function approach. I am very concerned with the increased use of inject() and now the deprecation of the class based route guard strategy.

I will let other more intelligent people comment on the more technical pros and cons of function based versus class based. I would like to speak up more about the art of what we do. I enjoy class based thinking, designing, and coding. Thats me. I know others enjoy a more functional based. More power to ya. But I grew up in the era of function based coding and it always felt like pages ripped out of a book. Never feeling like a well designed collection of pages. A scratch pad versus a book. Class based thinking and coding changed that for me. I absolutely love it.

So when it came to deciding between React or Angular, I fell in love with Angular because of its traditional stance that the class approach was its way. Whereas, React was moving strongly in the functional approach.

It worries me that Angular seems to be moving away from where it started. Things like stand alone and the down playing of modules and now the route guard. In my opinion, I have invested lots of time learning the spirit of Angular and fell in love with modules and classes.

I just wanted to speak up and say that there is at least one guy here that had no problem with the way you had it. And I have no problem with boilerplate if it is consistent, easy to understand, and well documented.

@bilgino
Copy link

bilgino commented Feb 28, 2023

Very disappointing change! The class-based approach and constructor injection was explicit and expressive with a good contract design. Consistency is most important!

I don't get the point, why it is not possible to make this feature optional? Because of bundle size? We are installing tons of 3rd-party libraries into our angular applications. Who cares about 2kb extra?

We are now starting the cycling from scratch. Don't see much difference to requireJS library from the old days.

require(["modules/ajax", "modules/router"], function(ajax, router) {});

compared to...

function myGuardWithMockableDeps(
dep1 = inject(MyService),
dep2 = inject(MyService2),
dep3 = inject(MyService3),
) { }

Indeed, this will lead developers to inject dependencies all over the place by using inject() here and inject() there...

... what a nightmare.

@michaelfaith
Copy link

constructor injection was explicit and expressive with a good contract design.

Honestly, not really. This is one of the more confusing concepts to teach new Angular devs, in my experience. Most other frameworks that use dependency injection (e.g. Spring) don't do it via constructor params. It wasn't explicit at all. I mean, we've all gotten used to it, and it's become second nature, but for people new to Angular, that's not as much the case. The inject api is much more explicit. You're specifically saying "i want to inject this thing", rather than just putting it as a parameter in some class' constructor.

@thomas-w-grant
Copy link

I come from a very long career in .Net. In the last 5-6 years I have switched over to Angular development. Angular made absolute sense to me. I fell in love. ❤️

That's nice Tom, what is your point? My point is that this stuff is subjective for the most part.

Subjectively, my truth is that constructor injection and class based development are superior. It is the "thing" that made Angular easy to learn. I suppose my .Net experience had a big part in forming my opinions around this. But these are true for me. And I would imagine true for lots of folks.

Does that mean it is superior for everyone. Absolutely not. And by the very same argument, it is also true that constructor injection and class based development is not inferior either. I do not wish to make a case @michaelfaith that you are wrong in your opinion. I just want to make a case that I am not either.

So what it really boils down to is justification for deprecation. In my opinion, deprecation is a much more delicate and serious matter than adding new features. You are changing an established contract. Hard working folks have invested time and energy to learn and write Angular code. I have invested over 5 years diving deep into the inner workings of Angular so I could bring more value to the folks paying me. I think having these things deprecated warrants more than presenting a subjective opinion.

This PR was created based on the following argument:

"Class and InjectionToken-based guards and resolvers are not as configurable,
are less re-usable, require more boilerplate, cannot be defined inline with the route,
and require more in-depth knowledge of Angular features (Injectable/providers).
In short, they're less powerful and more cumbersome."

That is a subjective statement. How do I know that? Because it is not true for me. That is true for another person. Objective means that it is true for all. Kind of like the statement "Diet Pepsi is superior over Diet Coke". You can't say that is objective. But for me, it is the truth. But I would never want Diet Coke to be cancelled.

Subjectively, I had no issue whatsoever learning/understanding constructor dependency injection. It would not be objective of me to say that it is easy to learn. It was easy to learn for me. It is what I have known for 20 years. Of course it would be easier to learn. The same could be said if you found something hard to learn. That may be true for you but it does not mean the material is hard to learn. It is hard to learn for your student.

I find issue with a decision of removing something that was established quite a while ago based on a subjective statement. I have no issue (indifferent really) with a new feature being added to allow folks to use the function based. I guess I am just asking for the same courtesy back that you not remove something that I value. I wouldn't block you, why are you so willing to block me.

A secondary issue that I have can maybe be best stated with a question: how much can you change before the thing is no longer the thing. For me, I fell in love with Angular because it presented a very familiar approach to dependency injection and class based development. I also liked the idea of separate html, css, and ts files. I know that Angular is so much more than that. But fundamentally for me, that is Angular. You change any of those things, and it is no longer Angular to me.

I guess some folks come to Angular and didn't love it as much as I did. Does that warrant changing Angular to something else? In my opinion, no. React is an awesome framework that embraces a more functional based development methodology. I wouldn't go over to the React community and start demanding that they change to separate file approach and class based.

Probably the final argument made against what I have said above is that if the majority want Angular to change, Angular is going to change. I respect that. I am just making sure my voice is counted when establishing where the true majority resides on these issues.

@KPLK

This comment was marked as abuse.

@mgechev
Copy link
Member

mgechev commented Mar 13, 2023

Hey folks, I want to be clear that we’re not removing the ability to author class-based guards and resolvers - we're adding an alternative way to develop guards and resolvers with functions. Here are the helper functions that allow you to use your class-based guards and resolvers moving forward.

There are plenty of benefits of the functional approach, as @atscott mentioned, and I also agree with some of the comments about the advantages of class-based guards and resolvers. Everything in engineering has its trade-offs. That's why we have functional programming languages like Haskell and Scala and object-oriented languages like Java and C#. You can also notice that object-oriented languages evolve and add more functional concepts over time, such as lambda functions. There's space for all these ideas and paradigms.

We're making trade-offs here to support a more expressive approach - we have two ways of doing things. People can now use both classes and functions for guards and resolvers. This will likely be the case until there's a strong signal that most of the community moves to one of the directions or there are very strong technical arguments that we have to stop supporting one of these approaches.

I also wanted to give everyone a heads up that @atscott is working on a code transformation that will wrap your classes so that your apps continue to function the way they do today without any manual change you have to do. On top of this, @atscott is testing the change and the code transformation on more than 50 million lines of code, in apps such as Google Analytics, and Google Cloud Console, to ensure everything works as expected.

I'm excited about the infrastructure and expertise we have to move the framework forward and evolve everyone's apps together with it.

@ShawnTalbert
Copy link

constructor injection was explicit and expressive with a good contract design.

Honestly, not really. This is one of the more confusing concepts to teach new Angular devs, in my experience. Most other frameworks that use dependency injection (e.g. Spring) don't do it via constructor params. It wasn't explicit at all. I mean, we've all gotten used to it, and it's become second nature, but for people new to Angular, that's not as much the case. The inject api is much more explicit. You're specifically saying "i want to inject this thing", rather than just putting it as a parameter in some class' constructor.

Constructor injection has been around for decades, and indeed is the preferred technique in several DI containers. It's a successful pattern used across some of the most popular languages/platforms.

It's been years since I used Spring but I'm pretty sure it supported constructor injection just fine? Even without 'DI' in mind, constructor parameters have been the first choice for providing whatever a class needs to successfully create an instance.

In any case, I love hybrid functional OO development (yay Scala) but I'm not seeing any tangible evidence that this functional style is better, other than the 'boilerplate' claim (though that is only a 2 or 3 lines of code?). Maybe @atscott can elaborate on specific advantages?

For example off the top of my head, I'd think a class based approach is more configurable than a plain function - in that a class can have whatever else you need neatly encapsulated within, exposing just the required method(s).

@ShawnTalbert
Copy link

According to the docs class guards are deprecated and it's clear there is a preference suddenly for the functional style. According to the link, class based guards could be removed in v17?

@MattiJarvinen
Copy link

According to the docs class guards are deprecated and it's clear there is a preference suddenly for the functional style. According to the link, class based guards could be removed in v17?

@mgechev so which one is it? Deprecated and removed or like you stated above just another way to do this?

@TCB13
Copy link

TCB13 commented Apr 26, 2023

I want to be clear that we’re not removing the ability to author class-based guards and resolvers

No, you just made it so hard that nobody will use class-based guards anymore. Frankly the best thing about angular was structure and what you've done here with these new approaches is making Angular less predictable and harder to scale.

No documentation is provided into the helper functions.

@crush83
Copy link

crush83 commented May 2, 2023

This direction is extremely unsettling. Many well-described comments have been just flat out ignored, and the Angular team just continues trudging forward with such an absolutely controversial change. This pull request closed and merged into main even...what is driving this direction?

Dependency injection via constructor is a long accepted pattern written, encouraged, and instructed by the brightest minds in software architecture for literal decades. I'd challenge you to find a single one of them who would vouch for this new design choice. This is a concept that transcends language and platform boundaries.

This new functional approach is encouraging hidden complexity, a code smell and design anti-pattern. It clearly violates the Principle of Least Surprise. It is unclear to the caller of the function which dependencies are required to run the function until it fails at runtime. Developers now need to dive into a function's implementation in order to see what dependencies must be wired into Angular's injection framework before calling the function.

How is that not a massive red flag that this is an anti-pattern? How is anyone advocating this approach?

The "helper" functions cleverly tuck away the problem, but also negate any performance gains that would be seen by using the functional approach...

I have yet to see a coherent argument in favor of the deprecation of the class-based guards, and I know I won't see one for reasons aforementioned, beyond a change in style preference by the Angular team.

My team has been using Angular since version 4 in all our projects. However, if this continues, we will be pursuing a new direction for sure. Maybe the Angular team just doesn't care. It sure seems that way from the way this discussion has been engaged.

I rarely engage in these comments, but this is just unacceptable in my opinion.

@mgechev As others have noted, and you've yet to address, if you aren't removing class-based guards and resolvers, why have they been marked as deprecated?

Edit

Just as it is a code smell and discouraged to inject an Injector instance into a class and utilize it within the class to resolve dependencies, it is a code smell to use an ambiently available inject function to inject dependencies within the function.

For example, you wouldn't write the following:

export class ShoppingCartComponent {
    constructor(private readonly injector: Injector) {}

    submitOrder() {
        const service = this.injector.get<OrderService>();
        service.placeOrder(order);
     }
}

This would introduce hidden complexity because the caller of the function does not know what services must be wired up to the injection container until their usage fails at runtime. The compiler can offer no aid to the developer to hint that a specific dependency is required. Unit testing becomes brittle and more time consuming.

Curious to know if there is an impact on tree shaking of these dependencies resolved this way?

I just can't help but think that if the Angular team is moving in this direction for route guards, what's next? Will we see more and more examples utilizing this ambient inject function to resolve dependencies? How is this different than the Service Locator anti-pattern?

@TCB13
Copy link

TCB13 commented May 2, 2023

@mgechev As others have noted, and you've yet to address, if you aren't removing class-based guards and resolvers, why have they been marked as deprecated?

That is what I've missed in my post. Thank you :)

@mgechev
Copy link
Member

mgechev commented May 3, 2023

I'll be unavailable for the next 10 days, but will make sure I expand more when I'm back.

@mgechev so which one is it? Deprecated and removed or like you stated above just another way to do this?

The tldr; is that using classes is still an option via the functional guard helpers we shared. We deprecated the interfaces because we're getting type checking that the classes implement the correct type based on the type definitions of the helpers.

When I'm back in office I'll follow up and we can discuss what's the right way to address your concerns. I'll be happy to schedule a livestream or a conversation in an alternative format, if needed.

@TCB13
Copy link

TCB13 commented May 3, 2023

The tldr; is that using classes is still an option via the functional guard helpers we shared.

Previously, Angular provided a strong and scalable base structure. The helpers are simply quick polyfills made to allow older code compatible with newer versions of Angular. This change will likely result in class-based approaches no longer being a first-choice option for developers.

This new approach falls to ground very quickly as one-line resolvers are very rare. In most applications resolvers are usually filled with RxJS code and can quickly become 10-20 lines long. Why would we want that in your routes? While it's important to make things easier for newcomers, it shouldn't be at the expense of killing Angular's best selling points - its scalability. This will most likely lead to new projects becoming hard to manage piles of code.

Class-based approaches should be the standard way, allowing for decent decoupling and re-use of logic, rather than relying on obscure polyfills.

@JeanMeche
Copy link
Member

This new approach falls to ground very quickly as one-line resolvers are very rare. In most applications resolvers are usually filled with RxJS code and can quickly become 10-20 lines long. Why would we want that in your routes?

The trick is that they don't have to be.
Functions don't mean inline functions.

export class MyGuard {

}

becomes

export function MyGuard {}

You still have the decoupling without the boiler plate of a class that is only instantiated for a method call.

@TCB13
Copy link

TCB13 commented May 3, 2023

Functions don't mean inline functions.

It doesn't really matter if it is really inline or exported from another file.

With this you lose a lot of things, eg. proper dependency management via constructor based injection. As @crush83 said we're just throwing away solid practices, decades of software engineering, just because someone feels like it.

the boiler plate of a class

I don't see this as a problem, as Angular devs have been using generators and schematics for quite some time now. Furthermore, IDEs such as WebStorm provide automation for everything. So unless you're coding in Notepad, it shouldn't be an issue.

Performance-wise, I would prefer to have that so-called 'boilerplate' code around, as it helps ensure that apps don't become unmanageable and that we have proper dependency injection. If someone were to benchmark this, it's likely they would find that the impact of these classes is negligible.

@oliveti
Copy link

oliveti commented May 10, 2023

Two points about interfaces deprecation :

Migration and type checking

The tldr; is that using classes is still an option via the functional guard helpers we shared. We deprecated the interfaces because we're getting type checking that the classes implement the correct type based on the type definitions of the helpers.

I think, removing the interface implementation pattern will complexify a lot automatic migrations in the future. For instance, what will happen if the method profile of canActivate guards changes and angular wants to provide an automatic migration via a schematic ?

If the functional guard is used in the current project, that's feasible. But, what if it is not used anywhere (released as part of a library for instance) ? Then how the schematics would identify the function as a guard ?

Usability

Also, but that's a minor point, interfaces are providing a nice developer experience. Without interfaces there is no way to :

  • Scaffold code to implement the guard.
  • Have an overview of all the guards of a project in the IDE.
  • Have self contained type definition, it requires to reference the guard to ensure the correct type (useful at development time if you create a guard and unit tests before using it in the project).

@DaSchTour
Copy link

Just my 2 cents of what happend here. The automatic removal of the interfaces in Angular 16 forced me to halt the update until we have migrated all guards and resolvers to the new pattern. Which is a pitty because we could already be using new Angular 16 features while doing the migration step by step. I fell that this was a bit rushed. I fear that a lot of projects will remain on Angular 15 for this reason.

@mgechev
Copy link
Member

mgechev commented May 17, 2023

Hey folks, I'd be happy to schedule a call to discuss functional router guards, continuing to use classes, etc. Would people who want to participate send me an email at mgechev at google dot com.

@linuxninja39
Copy link

I like to simply add my voice to many in this thread. There have been many very solid, principle based arguments in favor of keeping class based guards. Moving to function guard AND depreciating class based guards feels like an extremely unilateral decision. I have no objection to a function based guard as an OPTION, but, I'm extremely frustrated being pushed in to this, what I could consider very broken, paradigm. It feel very anti-pattern to what angular is. Did the developer come from a React team?

@TCB13
Copy link

TCB13 commented May 31, 2023

It feel very anti-pattern to what angular is. Did the developer come from a React team?

Ahaha it does seem like it. It also seems like someone at Angular/Google decided to simply expand their target audience by appealing to the React people. Engineers, predictability, large applications and whatnot are not a main concern anymore.

It is sad, but that is what it is. That is just Google, every new team which arrives have “different vision” and rest of us get unnecessary tech debt - the price of using Google stuff.

@e-oz
Copy link

e-oz commented May 31, 2023

The Father of Angular has built a react-like framework (with JSX, hooks), so I assume it affects teams' “vision”. Misko is also a big proponent of Signals.

@earbullet
Copy link

As a part of this change to preferring functions, can the example for function based be updated to be more realistic? At a minimum it should show how to access the things you could easily access before. Referring to the example on this page: https://angular.io/api/router/CanDeactivateFn

Everyone is going to write a generic function / class to check for unsaved changes, open a dialog to prompt the user (async), and need to inject the things they injected before (component, ActivatedRouteSnapshot, RouterStateSnapshot (current), RouterStateSnapshot (next))

With the inline function, every line will be custom in the route table; whereas, before it was all consistent or basically decorated. This is the unsavory part.

canDeactivate: [(component: ComponentX, ...) => inject(UnsavedChangesGuardService).canDeactivate(component, ...)]
canDeactivate: [(component: ComponentY, ...) => inject(UnsavedChangesGuardService).canDeactivate(component, ...)]
canDeactivate: [(component: ComponentZ, ...) => inject(UnsavedChangesGuardService).canDeactivate(component, ...)]

code that appears many times before

canDeactivate: [UnsavedChangesGuardService]
canDeactivate: [UnsavedChangesGuardService]
canDeactivate: [UnsavedChangesGuardService]

It's good that I can do constructor injection but most of what this reusable guard function needs is not able to be injected as it is currently working (or I cannot figure it out).

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview detected: deprecation PR contains a commit with a deprecation target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet