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

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

Closed

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Oct 24, 2018

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.

@mary-poppins
Copy link

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

Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

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/compiler": "0.0.0-PLACEHOLDER",

Copy link
Contributor

@IgorMinar IgorMinar left a comment

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.

packages/compiler/src/util.ts Show resolved Hide resolved
packages/core/BUILD.bazel Show resolved Hide resolved
packages/core/src/render3/jit/compiler_facade.ts Outdated Show resolved Hide resolved
packages/compiler/src/jit_compiler_facade.ts Outdated Show resolved Hide resolved
packages/core/src/render3/jit/compiler_facade.ts Outdated Show resolved Hide resolved
Copy link
Member

@alxhub alxhub left a comment

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.

packages/core/src/render3/jit/compiler_facade_interface.ts Outdated Show resolved Hide resolved
packages/core/src/render3/jit/compiler_facade.ts Outdated Show resolved Hide resolved
@alexeagle
Copy link
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.

@mary-poppins
Copy link

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
Copy link

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

@mary-poppins
Copy link

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
Copy link

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
Copy link
Contributor Author

mhevery commented Oct 29, 2018

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

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

@mhevery
Copy link
Contributor Author

mhevery commented Oct 30, 2018

@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
Copy link

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

@mary-poppins
Copy link

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
Copy link
Contributor Author

mhevery commented Oct 30, 2018

@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 added a commit to mhevery/angular that referenced this pull request Oct 31, 2018
gkalpak pushed a commit to gkalpak/angular that referenced this pull request Oct 31, 2018
gkalpak pushed a commit to gkalpak/angular that referenced this pull request Oct 31, 2018
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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 14, 2019
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 cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants