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(ivy): ensure DebugNode/DebugElement are tree-shakeable in Ivy #35003

Closed
wants to merge 4 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jan 28, 2020

There are different DebugNode/DebugElement implementations (and associated helper functions) for ViewEngine and Ivy. Additionally, these classes/functions, which are defined inside the core package, are imported by the platform-browser package.

Previously, this code was not tree-shaken as expected in Ivy. #30130 partially addressed the issue, but only for the case where core and platform-browser end up in the same closure after webpack's scope hoisting. In cases where this is not the case, our webpack/terser based tooling is not capable of tree-shaking it.

This commit fixes the problem, by ensuring that the code retained in Ivy mode (due to the cross-package import) does not unnecessarily reference DebugNode/DebugElement, allowing the code to be tree-shaken away. This results in a 7.6KB reduction in the size of the main angular.io
bundle.

Jira issue: FW-1802

@mary-poppins
Copy link

You can preview a50c40e at https://pr35003-a50c40e.ngbuilds.io/.

@gkalpak gkalpak changed the title Fix remove debug element fix(ivy): ensure DebugNode/DebugElement are tree-shakeable in Ivy Jan 28, 2020
@gkalpak gkalpak added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix refactoring Issue that involves refactoring or code-cleanup labels Jan 28, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 28, 2020
@mary-poppins
Copy link

You can preview 02e8ef6 at https://pr35003-02e8ef6.ngbuilds.io/.

return null;
}

export const getDebugNodeR2: (nativeNode: any) => DebugNode | null = getDebugNodeR2__PRE_R3__;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what would be an appropriate name 😇

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@mary-poppins
Copy link

You can preview 3fccd85 at https://pr35003-3fccd85.ngbuilds.io/.

There are different `DebugNode`/`DebugElement` implementations (and
associated helper functions) for ViewEngine and Ivy. Additionally, these
classes/functions, which are defined inside the `core` package, are
imported by the `platform-browser` package.

Previously, this code was not tree-shaken as expected in Ivy. angular#30130
partially addressed the issue, but only for the case where `core` and
`platform-browser` end up in the same closure after webpack's scope
hoisting. In cases where this is not the case, our webpack/terser based
tooling is not capable of tree-shaking it.

This commit fixes the problem, by ensuring that the code retained in Ivy
mode (due to the cross-package import) does not unnecessarily reference
`DebugNode`/`DebugElement`, allowing the code to be tree-shaken away.
This results in a 7.6KB reduction in the size of the main angular.io
bundle.

Jira issue: [FW-1802](https://angular-team.atlassian.net/browse/FW-1802)
@mary-poppins
Copy link

You can preview 9c7d3dd at https://pr35003-9c7d3dd.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review January 28, 2020 17:12
@gkalpak gkalpak requested review from a team as code owners January 28, 2020 17:12
@gkalpak gkalpak requested a review from a team January 28, 2020 17:12
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.

George, you rock!

thanks for figuring this out.

❤️

@@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2987,
"main-es2015": 456604,
"main-es2015": 448956,
Copy link
Contributor

Choose a reason for hiding this comment

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

woot!!!

@@ -39,7 +39,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 254657,
"main-es2015": 247225,
Copy link
Contributor

Choose a reason for hiding this comment

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

sweet!

return null;
}

export const getDebugNodeR2: (nativeNode: any) => DebugNode | null = getDebugNodeR2__PRE_R3__;
Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 28, 2020
@IgorMinar IgorMinar removed the request for review from a team January 28, 2020 21:19
@IgorMinar IgorMinar removed the request for review from a team January 28, 2020 21:19
AndrewKushnir pushed a commit that referenced this pull request Jan 28, 2020
AndrewKushnir pushed a commit that referenced this pull request Jan 28, 2020
…#35003)

There are different `DebugNode`/`DebugElement` implementations (and
associated helper functions) for ViewEngine and Ivy. Additionally, these
classes/functions, which are defined inside the `core` package, are
imported by the `platform-browser` package.

Previously, this code was not tree-shaken as expected in Ivy. #30130
partially addressed the issue, but only for the case where `core` and
`platform-browser` end up in the same closure after webpack's scope
hoisting. In cases where this is not the case, our webpack/terser based
tooling is not capable of tree-shaking it.

This commit fixes the problem, by ensuring that the code retained in Ivy
mode (due to the cross-package import) does not unnecessarily reference
`DebugNode`/`DebugElement`, allowing the code to be tree-shaken away.
This results in a 7.6KB reduction in the size of the main angular.io
bundle.

Jira issue: [FW-1802](https://angular-team.atlassian.net/browse/FW-1802)

PR Close #35003
AndrewKushnir pushed a commit that referenced this pull request Jan 28, 2020
AndrewKushnir pushed a commit that referenced this pull request Jan 28, 2020
AndrewKushnir pushed a commit that referenced this pull request Jan 28, 2020
…#35003)

There are different `DebugNode`/`DebugElement` implementations (and
associated helper functions) for ViewEngine and Ivy. Additionally, these
classes/functions, which are defined inside the `core` package, are
imported by the `platform-browser` package.

Previously, this code was not tree-shaken as expected in Ivy. #30130
partially addressed the issue, but only for the case where `core` and
`platform-browser` end up in the same closure after webpack's scope
hoisting. In cases where this is not the case, our webpack/terser based
tooling is not capable of tree-shaking it.

This commit fixes the problem, by ensuring that the code retained in Ivy
mode (due to the cross-package import) does not unnecessarily reference
`DebugNode`/`DebugElement`, allowing the code to be tree-shaken away.
This results in a 7.6KB reduction in the size of the main angular.io
bundle.

Jira issue: [FW-1802](https://angular-team.atlassian.net/browse/FW-1802)

PR Close #35003
@gkalpak gkalpak deleted the fix-remove-DebugElement branch January 29, 2020 10:01
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
…angular#35003)

There are different `DebugNode`/`DebugElement` implementations (and
associated helper functions) for ViewEngine and Ivy. Additionally, these
classes/functions, which are defined inside the `core` package, are
imported by the `platform-browser` package.

Previously, this code was not tree-shaken as expected in Ivy. angular#30130
partially addressed the issue, but only for the case where `core` and
`platform-browser` end up in the same closure after webpack's scope
hoisting. In cases where this is not the case, our webpack/terser based
tooling is not capable of tree-shaking it.

This commit fixes the problem, by ensuring that the code retained in Ivy
mode (due to the cross-package import) does not unnecessarily reference
`DebugNode`/`DebugElement`, allowing the code to be tree-shaken away.
This results in a 7.6KB reduction in the size of the main angular.io
bundle.

Jira issue: [FW-1802](https://angular-team.atlassian.net/browse/FW-1802)

PR Close angular#35003
@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 Feb 29, 2020
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 area: core Issues related to the framework runtime cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants