Skip to content

fix(core): remove static dependency from @angular/core to @angular/compiler - #26734

Closed
mhevery wants to merge 1 commit into
angular:masterfrom
mhevery:core_does_not_depend_on_compiler
Closed

fix(core): remove static dependency from @angular/core to @angular/compiler#26734
mhevery wants to merge 1 commit into
angular:masterfrom
mhevery:core_does_not_depend_on_compiler

Conversation

@mhevery

@mhevery mhevery commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

With Ivy we need @Component decorator to compile the component. For this reason @Component depends on @angular/compiler. We expect that the the compiler will than be tree-shaken away as part of the production build.

Unfortunately the @angular/compiler is large enough that the mare act of parsing and loading (even if not executing) is causing negative build time regression. This change fixes this by making the dependency between @Component decorator and @angular/compiler to be late bound (runtime) rather than statically bound (compile time).

This means that for JIT mode it is not sufficient to just load @angular/core to have @angular/compiler loaded. Instead it is necessary to load @angular/compiler either explicitly or implicitly thought @angular/platform-browser-dynamic.

@mhevery
mhevery requested review from IgorMinar and alxhub October 24, 2018 23:11
@mary-poppins

mary-poppins commented Oct 24, 2018

Copy link
Copy Markdown

You can preview 8f6091c at https://pr26734-8f6091c.ngbuilds.io/.

alan-agius4
alan-agius4 reviewed Oct 25, 2018
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the peerDependency for compiler be dropped?

angular/packages/core/package.json

Line 19 in 9dc52d9

"@angular/compiler": "0.0.0-PLACEHOLDER",

IgorMinar
IgorMinar previously requested changes Oct 25, 2018
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

some changes requested but the rest looks good.

Comment thread packages/compiler/src/util.ts Resolved
Comment thread packages/core/BUILD.bazel Resolved
Comment thread packages/core/src/render3/jit/compiler_facade.ts Resolved
Comment thread packages/compiler/src/jit_compiler_facade.ts Resolved
Comment thread packages/core/src/render3/jit/compiler_facade.ts Resolved
alxhub
alxhub approved these changes Oct 25, 2018
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very cool! A few minor comments.

Comment thread packages/compiler/src/compiler_facade_interface.ts Resolved
Comment thread packages/core/src/render3/jit/compiler_facade_interface.ts Resolved
Comment thread packages/core/src/render3/jit/compiler_facade.ts Resolved
@alexeagle

alexeagle commented Oct 25, 2018

Copy link
Copy Markdown
Contributor

Can this PR be merged to patch and go out in the next 7.0.x release? Otherwise CLI needs to do more work to compensate.

@alexeagle alexeagle mentioned this pull request Oct 25, 2018
Closed
@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch 2 times, most recently from d90aefc to 9ad8478 Compare October 25, 2018 18:44
@mary-poppins

mary-poppins commented Oct 25, 2018

Copy link
Copy Markdown

You can preview 9ad8478 at https://pr26734-9ad8478.ngbuilds.io/.

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch 2 times, most recently from dbc2960 to c0b56ff Compare October 25, 2018 23:59
@mary-poppins

mary-poppins commented Oct 26, 2018

Copy link
Copy Markdown

You can preview c0b56ff at https://pr26734-c0b56ff.ngbuilds.io/.

@mary-poppins

mary-poppins commented Oct 26, 2018

Copy link
Copy Markdown

You can preview 57f3dec at https://pr26734-57f3dec.ngbuilds.io/.

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from 57f3dec to 029d8fb Compare October 26, 2018 18:48
@mhevery
mhevery dismissed IgorMinar’s stale review October 29, 2018 17:27

Concernes addressed.

@mary-poppins

mary-poppins commented Oct 29, 2018

Copy link
Copy Markdown

You can preview 63b9af5 at https://pr26734-63b9af5.ngbuilds.io/.

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from 63b9af5 to 0fe5d1a Compare October 29, 2018 21:42
@mhevery

mhevery commented Oct 29, 2018

Copy link
Copy Markdown
Contributor Author

http://test/OCL:219198443:BASE:219198494:1540850096541:6a343517

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from 0fe5d1a to db531c9 Compare October 29, 2018 22:00
@mary-poppins

mary-poppins commented Oct 29, 2018

Copy link
Copy Markdown

You can preview 98f38b6 at https://pr26734-98f38b6.ngbuilds.io/.

@trotyl trotyl mentioned this pull request Oct 30, 2018
Merged
@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from 98f38b6 to 66ecc41 Compare October 30, 2018 17:24
@mhevery

mhevery commented Oct 30, 2018

Copy link
Copy Markdown
Contributor Author

http://test/OCL:219198443:BASE:219325227:1540921167060:d9fb4894

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from 66ecc41 to 8cf90f4 Compare October 30, 2018 17:41
@mhevery mhevery added the target: major This PR is targeted for the next major release label Oct 30, 2018
@mary-poppins

mary-poppins commented Oct 30, 2018

Copy link
Copy Markdown

You can preview 8cf90f4 at https://pr26734-8cf90f4.ngbuilds.io/.

@mary-poppins

mary-poppins commented Oct 30, 2018

Copy link
Copy Markdown

You can preview 5d257f6 at https://pr26734-5d257f6.ngbuilds.io/.

@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Oct 30, 2018
@mhevery

mhevery commented Oct 30, 2018

Copy link
Copy Markdown
Contributor Author

http://test/OCL:219198443:BASE:219365318:1540934567758:ee4f97b

@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch 3 times, most recently from 4172b5f to bb6a3ed Compare October 30, 2018 23:18
@mhevery
mhevery force-pushed the core_does_not_depend_on_compiler branch from bb6a3ed to c4f71ff Compare October 30, 2018 23:23
@matsko matsko closed this in d042c4a Oct 31, 2018
mhevery added a commit to mhevery/angular that referenced this pull request Oct 31, 2018
@mhevery
fix(core): Remove static dependency from @angular/core to @angular/co…
b3a68d6 
…mpiler (angular#26734)

PR Close angular#26734
mhevery added a commit to mhevery/angular that referenced this pull request Oct 31, 2018
@mhevery
fixup! fix(core): Remove static dependency from @angular/core to @ang…
6078934 
…ular/compiler (angular#26734) PR Close angular#26734
mhevery added a commit that referenced this pull request Oct 31, 2018
@mhevery
fix(core): Remove static dependency from @angular/core to @angular/co…
257ac83 
…mpiler (#26734) (#26879)
@JoostK JoostK mentioned this pull request Dec 1, 2018
Closed
2 of 4 tasks
@filipesilva filipesilva mentioned this pull request Dec 27, 2018
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alan-agius4 alan-agius4 alan-agius4 left review comments
@alxhub alxhub alxhub approved these changes
+2 more reviewers
@alfaproject alfaproject alfaproject left review comments
@IgorMinar IgorMinar IgorMinar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker 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.

8 participants

@mhevery @mary-poppins @alexeagle @alfaproject @IgorMinar @alxhub @alan-agius4 @googlebot