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

feat: tree-shakeable providers API updates #22655

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@alxhub
Copy link
Contributor

commented Mar 8, 2018

Rename @Injectable({scope -> providedIn}).

Instead of {providedIn: APP_ROOT_SCOPE}, accept {providedIn: 'root'}.
Also, {providedIn: null} implies the injectable should not be added
to any scope.

@googlebot googlebot added the cla: yes label Mar 8, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 8, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 679fee0 to ae6e61c Mar 8, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 8, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from ae6e61c to 0210da8 Mar 8, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 8, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 0210da8 to 57d4702 Mar 8, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 8, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 57d4702 to 142c604 Mar 9, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 9, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 142c604 to 4bbb8b3 Mar 9, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 9, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch 2 times, most recently from cdd555b to 76207e5 Mar 12, 2018

@alxhub alxhub changed the title refactor: tree-shakeable providers API updates feat: tree-shakeable providers API updates Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 76207e5 to 5d277a6 Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 5d277a6 to e4bbad3 Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from e4bbad3 to af9c32f Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alxhub alxhub requested review from mhevery and chuckjaz Mar 12, 2018

@@ -127,7 +127,7 @@ export interface ModuleWithProviders {
providers?: Provider[];
}
export interface Injectable {
scope?: Type|any;
providedIn?: Type|any;

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

Consider adding |'root'. Since it is any it is not necessary but it would be more clear that 'root' is special.

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 12, 2018

Author Contributor

Done.

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch 2 times, most recently from 5cd6b0c to 1649970 Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 1649970 to 227e098 Mar 12, 2018

feat: tree-shakeable providers API updates
Rename @Injectable({scope -> providedIn}).

Instead of {providedIn: APP_ROOT_SCOPE}, accept {providedIn: 'root'}.
Also, {providedIn: null} implies the injectable should not be added
to any scope.

@alxhub alxhub force-pushed the alxhub:injectable-def-changes branch from 227e098 to 663f1dc Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 12, 2018

@@ -71,7 +71,7 @@ export function _document(): any {
@NgModule({
providers: [
BROWSER_SANITIZATION_PROVIDERS,
{provide: APP_ROOT_SCOPE, useValue: true},
{provide: APP_ROOT, useValue: true},

This comment has been minimized.

Copy link
@mhevery

mhevery Mar 12, 2018

Member

Is there another way to mark a module as root without relying on APP_ROOT? I think the design would be nicer if this line would not exist.

We could test if we are root if:

  • There is no parent
  • The parent injector is NULL_INJECTOR
  • The parent injector is PlatformRef injector?

Alternatively we could have the boostrap method call createModule with some extra argument to designate that it should be root injector.

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 12, 2018

Author Contributor

This is an internal implementation detail only (APP_ROOT is not exposed to the user), so yes, we could have a different way of detecting it.

This current setup does have an advantage - because APP_ROOT is tied to BrowserModule in the existing system, it guarantees that the application-level services which Angular provides are in the "root scope" injector as well. This isn't necessarily the case if the user gets creative with injector structures.

@@ -47,7 +42,7 @@ export function moduleDef(providers: NgModuleProviderDef[]): NgModuleDefinition
let isRoot: boolean = false;
for (let i = 0; i < providers.length; i++) {
const provider = providers[i];
if (provider.token === APP_ROOT_SCOPE) {
if (provider.token === APP_ROOT) {

This comment has been minimized.

Copy link
@mhevery

mhevery Mar 12, 2018

Member

Is there another way to mark a module as root without relying on APP_ROOT?

We could have the boostrap method call createModule with some extra argument to designate that it should be root injector.

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 12, 2018

Author Contributor

Answered above.

*/
export const APP_ROOT_SCOPE: Type<any> = new InjectionToken<boolean>(
'The presence of this token marks an injector as being the root injector.') as any;
export const APP_ROOT = new InjectionToken<boolean>(

This comment has been minimized.

Copy link
@mhevery

mhevery Mar 12, 2018

Member

I would like to get rid of APP_ROOT. It is not public, but it sort of is since platform-browser has a special relationship with it. Which means that platform-browser is special and only it can create root injectors.

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 12, 2018

Author Contributor

I think this should be a separate discussion and should not block this PR which is blocking 6.0 RC. Since it is internal we can change the behavior before 6.0 final.

@kara kara closed this in db56836 Mar 13, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat: tree-shakeable providers API updates (angular#22655)
Rename @Injectable({scope -> providedIn}).

Instead of {providedIn: APP_ROOT_SCOPE}, accept {providedIn: 'root'}.
Also, {providedIn: null} implies the injectable should not be added
to any scope.

PR Close angular#22655

@trotyl trotyl referenced this pull request May 4, 2018

Closed

APP_ROOT_SCOPE token #22185

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.