Skip to content

feat: tree-shakeable providers API updates #22655

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: tree-shakeable providers API updates #22655

wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Mar 8, 2018 •
edited
Loading

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.

@mary-poppins
Copy link

You can preview 679fee0 at https://pr22655-679fee0.ngbuilds.io/.

@alexeagle alexeagle added the area: core Issues related to the framework runtime label Mar 8, 2018
@alxhub alxhub force-pushed the injectable-def-changes branch from 679fee0 to ae6e61c Compare March 8, 2018 17:57
@mary-poppins
Copy link

You can preview ae6e61c at https://pr22655-ae6e61c.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from ae6e61c to 0210da8 Compare March 8, 2018 23:07
@mary-poppins
Copy link

You can preview 0210da8 at https://pr22655-0210da8.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from 0210da8 to 57d4702 Compare March 8, 2018 23:21
@mary-poppins
Copy link

You can preview 57d4702 at https://pr22655-57d4702.ngbuilds.io/.

@CaerusKaru CaerusKaru mentioned this pull request Mar 9, 2018
Closed
@alxhub alxhub force-pushed the injectable-def-changes branch from 57d4702 to 142c604 Compare March 9, 2018 23:37
@mary-poppins
Copy link

You can preview 142c604 at https://pr22655-142c604.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from 142c604 to 4bbb8b3 Compare March 9, 2018 23:47
@mary-poppins
Copy link

You can preview 4bbb8b3 at https://pr22655-4bbb8b3.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch 2 times, most recently from cdd555b to 76207e5 Compare March 12, 2018 16:05
@alxhub alxhub changed the title refactor: tree-shakeable providers API updates feat: tree-shakeable providers API updates Mar 12, 2018
@mary-poppins
Copy link

You can preview 76207e5 at https://pr22655-76207e5.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from 76207e5 to 5d277a6 Compare March 12, 2018 17:03
@mary-poppins
Copy link

You can preview 5d277a6 at https://pr22655-5d277a6.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from 5d277a6 to e4bbad3 Compare March 12, 2018 17:40
@mary-poppins
Copy link

You can preview e4bbad3 at https://pr22655-e4bbad3.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from e4bbad3 to af9c32f Compare March 12, 2018 20:09
@mary-poppins
Copy link

You can preview af9c32f at https://pr22655-af9c32f.ngbuilds.io/.

@alxhub alxhub requested review from mhevery and chuckjaz March 12, 2018 21:50
chuckjaz
chuckjaz approved these changes Mar 12, 2018
View reviewed changes
packages/compiler/src/core.ts Outdated
@@ -127,7 +127,7 @@ export interface ModuleWithProviders {
providers?: Provider[];
}
export interface Injectable {
scope?: Type|any;
providedIn?: Type|any;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@alxhub alxhub force-pushed the injectable-def-changes branch 2 times, most recently from 5cd6b0c to 1649970 Compare March 12, 2018 22:01
@mary-poppins
Copy link

You can preview 5cd6b0c at https://pr22655-5cd6b0c.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 1649970 at https://pr22655-1649970.ngbuilds.io/.

@alxhub alxhub force-pushed the injectable-def-changes branch from 1649970 to 227e098 Compare March 12, 2018 22:21
@alxhub
feat: tree-shakeable providers API updates
663f1dc 
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 injectable-def-changes branch from 227e098 to 663f1dc Compare March 12, 2018 22:24
@mary-poppins
Copy link

You can preview 227e098 at https://pr22655-227e098.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 663f1dc at https://pr22655-663f1dc.ngbuilds.io/.

mhevery
mhevery suggested changes Mar 12, 2018
View reviewed changes
packages/platform-browser/src/browser.ts
@@ -71,7 +71,7 @@ export function _document(): any {
@NgModule({
providers: [
BROWSER_SANITIZATION_PROVIDERS,
{provide: APP_ROOT_SCOPE, useValue: true},
{provide: APP_ROOT, useValue: true},
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

packages/core/src/view/ng_module.ts
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Answered above.

packages/core/src/di/scope.ts
*/
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>(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 13, 2018
@kara kara closed this in db56836 Mar 13, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@alxhub @leo6104
feat: tree-shakeable providers API updates (angular#22655)
a72b039 
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 mentioned this pull request May 4, 2018
Closed
@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 Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@chuckjaz chuckjaz chuckjaz approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alxhub @mary-poppins @mhevery @chuckjaz @alexeagle @googlebot